Merge patch series "pylibfdt: Update the U-Boot version with changes from upstream dtc"

Alexey Charkov <[email protected]> says:

U-Boot's copy of the libfdt Python bindings in
scripts/dtc/pylibfdt/libfdt.i_shipped was last synced wholesale in 2019,
by commit 903fe17aa8c8 ("pylibfdt: Sync up with upstream"), against dtc
commit 430419c28100. Since then it has only picked up individual fixes,
so it has drifted a long way behind dtc, which is now at v1.8.1-19.

Tom asked for an audit of the divergence when reviewing the FdtSw growth
patch [1]; this is the result. Bring in every missing change from upstream
as an individual commit, referencing its upstream sibling.

This adds: hasprop() and setprop_bool(), get_path(),
Property.as_stringlist() and the as_*int*_list() accessors,
address_cells()/size_cells(), add_mem_rsv()/del_mem_rsv(), geometric FdtSw
buffer growth, a missing Py_INCREF on the Py_None returned by the
fdt_getprop() typemap, correct get_mem_rsv() results on current Python,
and the SWIG 4.3+ return-value compatibility shim.

Not included is dtc commit 5008d1d6a356 ("pylibfdt: Replace removed SWIG
Python 2 compatibility macros"), which already landed independently as
commit 527115ef6783 ("pylibfdt: Replace removed SWIG Python 2
compatibility macros").

The first two patches are preparatory: they take the parts of two upstream
commits that earlier partial imports left behind, so the rest apply
verbatim. The last patch adds test coverage, since none of these bindings
have an in-tree caller yet and dtc's own tests for them live in
tests/pylibfdt_tests.py, which U-Boot does not carry.

Residual differences from upstream left untouched:
 - the SPDX comment style;
 - the %begin block defining PY_SSIZE_T_CLEAN, which is U-Boot's variant
   of a fix dtc applies from its build system;
 - the two %include paths, which follow U-Boot's directory layout;
 - the retry loop in FdtSw.as_fdt(), from commit 211cfa503f6c ("libfdt:
   Detected out-of-space with fdt_finish()") - U-Boot only change

CI passes all green [2]

[1] https://lore.kernel.org/u-boot/[email protected]/
[2] https://git.u-boot-project.org/u-boot/contributors/alchark/u-boot/-/pipelines/1047

Link: https://lore.kernel.org/r/[email protected]

2 files changed, 380 insertions, 25 deletions

diff --git a/scripts/dtc/pylibfdt/libfdt.i_shipped b/scripts/dtc/pylibfdt/libfdt.i_shipped
index 326a15c7941..3d527dfe212 100644
--- a/scripts/dtc/pylibfdt/libfdt.i_shipped
+++ b/scripts/dtc/pylibfdt/libfdt.i_shipped
@@ -118,11 +118,14 @@ def check_err_null(val, quiet=()):
         FdtException if val indicates an error was reported and the error
         is not in @quiet.
     """
-    # Normally a list is returned which contains the data and its length.
-    # If we get just an integer error code, it means the function failed.
+    # Compatibility for SWIG v4.2 and earlier. SWIG 4.2 would drop the first
+    # item from the list if it was None, returning only the second item.
     if not isinstance(val, list):
-        if -val not in quiet:
-            raise FdtException(val)
+        val = [None, val]
+
+    if val[0] is None:
+        if -val[1] not in quiet:
+            raise FdtException(val[1])
     return val
 
 class FdtRo(object):
@@ -163,7 +166,7 @@ class FdtRo(object):
             quiet: Errors to ignore (empty to raise on all errors)
 
         Returns:
-            Typle:
+            Tuple:
                 Offset of the next node, if any, else a -ve error
                 Depth of the returned node, if any, else undefined
 
@@ -299,7 +302,41 @@ class FdtRo(object):
         Returns:
             Number of memory reserve-map records
         """
-        return check_err(fdt_get_mem_rsv(self._fdt, index), quiet)
+        val = fdt_get_mem_rsv(self._fdt, index)
+        check_err(val[0], quiet)
+        return val[1:]
+
+    def address_cells(self, nodeoffset, quiet=()):
+        """Return the number of address cells used by a node's children
+
+        Args:
+            nodeoffset: Offset of the node to check
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Number of address cells used by the children of @nodeoffset
+
+        Raises:
+            FdtException if the node has an invalid #address-cells property,
+            or another error occurs
+        """
+        return check_err(fdt_address_cells(self._fdt, nodeoffset), quiet)
+
+    def size_cells(self, nodeoffset, quiet=()):
+        """Return the number of size cells used by a node's children
+
+        Args:
+            nodeoffset: Offset of the node to check
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Number of size cells used by the children of @nodeoffset
+
+        Raises:
+            FdtException if the node has an invalid #size-cells property, or
+            another error occurs
+        """
+        return check_err(fdt_size_cells(self._fdt, nodeoffset), quiet)
 
     def subnode_offset(self, parentoffset, name, quiet=()):
         """Get the offset of a named subnode
@@ -397,8 +434,8 @@ class FdtRo(object):
         """
         pdata = check_err_null(
                 fdt_get_property_by_offset(self._fdt, prop_offset), quiet)
-        if isinstance(pdata, (int)):
-            return pdata
+        if pdata[0] is None:
+            return pdata[1]
         return Property(pdata[0], pdata[1])
 
     def getprop(self, nodeoffset, prop_name, quiet=()):
@@ -419,9 +456,38 @@ class FdtRo(object):
         """
         pdata = check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
                                quiet)
-        if isinstance(pdata, (int)):
-            return pdata
-        return Property(prop_name, bytes(pdata[0]))
+        if pdata[0] is None:
+            return pdata[1]
+        return Property(prop_name, bytearray(pdata[0]))
+
+    def hasprop(self, nodeoffset, prop_name, quiet=()):
+        """Check if a node has a property
+
+        This can be used to check boolean properties
+
+        Args:
+            nodeoffset: Node offset containing property to check
+            prop_name: Name of property to check
+            quiet: Errors to ignore (empty to raise on all errors). Note that
+                NOTFOUND is added internally by this function so need not be
+                provided
+
+        Returns:
+            True if the property exists in the node, else False. If an error
+                other than -NOTFOUND is returned by fdt_getprop() then the error
+                is return (-ve integer)
+
+        Raises:
+            FdtError if any error occurs other than NOTFOUND (e.g. the
+                nodeoffset is invalid)
+        """
+        pdata = check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
+                               quiet + (NOTFOUND,))
+        if pdata[0] is None:
+            if pdata[1] == -NOTFOUND:
+                return False
+            return pdata[1]
+        return True
 
     def get_phandle(self, nodeoffset):
         """Get the phandle of a node
@@ -447,6 +513,29 @@ class FdtRo(object):
         """
         return fdt_get_alias(self._fdt, name)
 
+    def get_path(self, nodeoffset, size_hint=1024, quiet=()):
+        """Get the full path of a node
+
+        Args:
+            nodeoffset: Node offset to check
+            size_hint: Hint for size of returned string
+
+        Returns:
+            Full path to the node
+
+        Raises:
+            FdtException if an error occurs
+        """
+        while True:
+            ret, path = fdt_get_path(self._fdt, nodeoffset, size_hint)
+            if ret == -NOSPACE:
+                size_hint *= 2
+                continue
+            err = check_err(ret, quiet)
+            if err:
+                return err
+            return path
+
     def parent_offset(self, nodeoffset, quiet=()):
         """Get the offset of a node's parent
 
@@ -551,6 +640,42 @@ class Fdt(FdtRo):
         del self._fdt[self.totalsize():]
         return err
 
+    def add_mem_rsv(self, addr, size, quiet=()):
+        """Add a memory reserve-map record
+
+        This asks the client program not to use the given region of memory,
+        e.g. because something was loaded there.
+
+        Args:
+            addr: Start address of the region to reserve
+            size: Size of the region to reserve, in bytes
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Error code, or 0 if OK
+
+        Raises:
+            FdtException if there is no space for another record, or another
+            error occurs
+        """
+        return check_err(fdt_add_mem_rsv(self._fdt, addr, size), quiet)
+
+    def del_mem_rsv(self, index, quiet=()):
+        """Remove the indexed memory reserve-map record
+
+        Args:
+            index: Record to remove (0=first)
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Error code, or 0 if OK
+
+        Raises:
+            FdtException if there is no record at @index, or another error
+            occurs
+        """
+        return check_err(fdt_del_mem_rsv(self._fdt, index), quiet)
+
     def set_name(self, nodeoffset, name, quiet=()):
         """Set the name of a node
 
@@ -586,6 +711,32 @@ class Fdt(FdtRo):
         return check_err(fdt_setprop(self._fdt, nodeoffset, prop_name, val,
                                      len(val)), quiet)
 
+    def setprop_bool(self, nodeoffset, prop_name, val, quiet=()):
+        """Set the boolean value of a property
+
+        Either:
+            adds the property if not already present; or
+            deletes the property if present
+
+        Args:
+            nodeoffset: Node offset containing the property to create/delete
+            prop_name: Name of property
+            val: Boolean value to write (i.e. True or False)
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Error code, or 0 if OK
+
+        Raises:
+            FdtException if no parent found or other error occurs
+        """
+        exists = self.hasprop(nodeoffset, prop_name, quiet)
+        if val != exists:
+            if val:
+                return self.setprop(nodeoffset, prop_name, b'', quiet=quiet)
+            else:
+                return self.delprop(nodeoffset, prop_name, quiet=quiet)
+
     def setprop_u32(self, nodeoffset, prop_name, val, quiet=()):
         """Set the value of a property
 
@@ -720,6 +871,21 @@ class Property(bytearray):
     def as_int64(self):
         return self.as_cell('q')
 
+    def as_list(self, fmt):
+        return list(map(lambda x: x[0], struct.iter_unpack('>' + fmt, self)))
+
+    def as_uint32_list(self):
+        return self.as_list('L')
+
+    def as_int32_list(self):
+        return self.as_list('l')
+
+    def as_uint64_list(self):
+        return self.as_list('Q')
+
+    def as_int64_list(self):
+        return self.as_list('q')
+
     def as_str(self):
         """Unicode is supported by decoding from UTF-8"""
         if self[-1] != 0:
@@ -728,6 +894,13 @@ class Property(bytearray):
             raise ValueError('Property contains embedded nul characters')
         return self[:-1].decode('utf-8')
 
+    def as_stringlist(self):
+        """Unicode is supported by decoding from UTF-8"""
+        if self[-1] != 0:
+            raise ValueError('Property lacks nul termination')
+        parts = self[:-1].split(b'\x00')
+        return list(map(lambda x: x.decode('utf-8'), parts))
+
 
 class FdtSw(FdtRo):
     """Software interface to create a device tree from scratch
@@ -756,7 +929,7 @@ class FdtSw(FdtRo):
     device tree. This will be increased automatically as needed as new items
     are added to the tree.
     """
-    INC_SIZE = 1024  # Expand size by this much when out of space
+    INC_SIZE = 1024  # Expand size by at least this much when out of space
 
     def __init__(self, size_hint=None):
         """Create a new FdtSw object
@@ -801,6 +974,10 @@ class FdtSw(FdtRo):
         -NOSPACE then the FDT will be expanded to have more space, and True will
         be returned, indicating that the operation needs to be tried again.
 
+        Each expansion copies the whole tree into a new buffer, so the size is
+        at least doubled rather than grown by a fixed amount, to keep the total
+        amount of copying proportional to the size of the tree.
+
         Args:
             val: Return value from the operation that was attempted
 
@@ -808,7 +985,7 @@ class FdtSw(FdtRo):
             True if the operation must be retried, else False
         """
         if check_err(val, QUIET_NOSPACE) < 0:
-            self.resize(len(self._fdt) + self.INC_SIZE)
+            self.resize(len(self._fdt) + max(len(self._fdt), self.INC_SIZE))
             return True
         return False
 
@@ -935,7 +1112,7 @@ class FdtSw(FdtRo):
 
         Args:
             name: Name of property to add
-            val: Value of property
+            val: Value of property (bytes)
             quiet: Errors to ignore (empty to raise on all errors)
 
         Raises:
@@ -996,6 +1173,9 @@ class NodeAdder():
 
 %rename(fdt_property) fdt_property_func;
 
+%immutable fdt_property::data;
+%immutable fdt_node_header::name;
+
 /*
  * fdt32_t is a big-endian 32-bit value defined to uint32_t in libfdt_env.h
  * so use the same type here.
@@ -1045,14 +1225,16 @@ typedef uint32_t fdt32_t;
 
 /* typemap used for fdt_getprop() */
 %typemap(out) (const void *) {
-	if (!$1)
+	if (!$1) {
 		$result = Py_None;
-	else
+		Py_INCREF($result);
+	} else {
         %#if PY_VERSION_HEX >= 0x03000000
-            $result = Py_BuildValue("y#", $1, *arg4);
+            $result = Py_BuildValue("y#", $1, (Py_ssize_t)*arg4);
         %#else
-            $result = Py_BuildValue("s#", $1, *arg4);
+            $result = Py_BuildValue("s#", $1, (Py_ssize_t)*arg4);
         %#endif
+    }
 }
 
 /* typemap used for fdt_setprop() */
@@ -1088,14 +1270,14 @@ typedef uint32_t fdt32_t;
 
 %typemap(argout) uint64_t * {
         PyObject *val = PyLong_FromUnsignedLongLong(*arg$argnum);
-        if (!result) {
-           if (PyTuple_GET_SIZE(resultobj) == 0)
-              resultobj = val;
-           else
-              resultobj = SWIG_AppendOutput(resultobj, val);
-        }
+        resultobj = SWIG_AppendOutput(resultobj, val);
 }
 
+%include "cstring.i"
+
+%cstring_output_maxsize(char *buf, int buflen);
+int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen);
+
 /* We have both struct fdt_property and a function fdt_property() */
 %warnfilter(302) fdt_property;
 
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index a858da127bf..66d346b1272 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -10,6 +10,7 @@ Written by Simon Glass <[email protected]>
 from argparse import ArgumentParser
 import os
 import shutil
+import struct
 import sys
 import tempfile
 import unittest
@@ -754,6 +755,178 @@ class TestProp(unittest.TestCase):
                          self.dtb.GetFilename())
 
 
+class TestPylibfdt(unittest.TestCase):
+    """Tests for the parts of pylibfdt that dtoc does not itself use
+
+    These bindings come from upstream dtc and have no coverage elsewhere in
+    U-Boot, so exercise them directly rather than through the Fdt wrapper.
+    """
+
+    @classmethod
+    def setUpClass(cls):
+        tools.prepare_output_dir(None)
+
+    @classmethod
+    def tearDownClass(cls):
+        tools.finalise_output_dir()
+
+    def setUp(self):
+        self.dtb = fdt.FdtScan(find_dtb_file('dtoc_test_simple.dts'))
+        self.fdt = self.dtb.GetFdtObj()
+        self.node = self.fdt.path_offset('/spl-test')
+
+    def test_hasprop(self):
+        """Test checking for the presence of a property"""
+        self.assertTrue(self.fdt.hasprop(self.node, 'boolval'))
+        self.assertTrue(self.fdt.hasprop(self.node, 'intval'))
+        self.assertFalse(self.fdt.hasprop(self.node, 'missing'))
+
+    def test_hasprop_bad_node(self):
+        """Test that hasprop() still reports errors other than NOTFOUND"""
+        with self.assertRaises(libfdt.FdtException) as exc:
+            self.fdt.hasprop(-1, 'boolval')
+        self.assertEqual(-libfdt.BADOFFSET, exc.exception.err)
+
+    def test_setprop_bool(self):
+        """Test creating and deleting a boolean property"""
+        # Leave room for the new property
+        self.fdt.resize(self.fdt.totalsize() + 1024)
+        node = self.fdt.path_offset('/spl-test')
+
+        self.fdt.setprop_bool(node, 'newbool', True)
+        self.assertTrue(self.fdt.hasprop(node, 'newbool'))
+        self.assertEqual(b'', self.fdt.getprop(node, 'newbool'))
+
+        # Setting it again should be a no-op
+        self.fdt.setprop_bool(node, 'newbool', True)
+        self.assertTrue(self.fdt.hasprop(node, 'newbool'))
+
+        self.fdt.setprop_bool(node, 'newbool', False)
+        self.assertFalse(self.fdt.hasprop(node, 'newbool'))
+
+        # Deleting it again should also be a no-op
+        self.fdt.setprop_bool(node, 'newbool', False)
+        self.assertFalse(self.fdt.hasprop(node, 'newbool'))
+
+    def test_get_path(self):
+        """Test reading back the full path of a node"""
+        node = self.fdt.path_offset('/i2c@0/pmic@9')
+        self.assertEqual('/i2c@0/pmic@9', self.fdt.get_path(node))
+        self.assertEqual('/', self.fdt.get_path(0))
+
+    def test_get_path_no_space(self):
+        """Test that get_path() retries with a larger buffer as needed"""
+        node = self.fdt.path_offset('/i2c@0/pmic@9')
+        self.assertEqual('/i2c@0/pmic@9', self.fdt.get_path(node, size_hint=1))
+
+    def test_get_path_bad_node(self):
+        """Test get_path() on an invalid node offset"""
+        with self.assertRaises(libfdt.FdtException) as exc:
+            self.fdt.get_path(-1)
+        self.assertEqual(-libfdt.BADOFFSET, exc.exception.err)
+
+    def test_as_stringlist(self):
+        """Test decoding a string-list property"""
+        prop = self.fdt.getprop(self.node, 'stringarray')
+        self.assertEqual(['multi-word', 'message'], prop.as_stringlist())
+
+        prop = self.fdt.getprop(self.node, 'stringval')
+        self.assertEqual(['message'], prop.as_stringlist())
+
+    def test_as_int_lists(self):
+        """Test decoding integer-array properties"""
+        prop = self.fdt.getprop(self.node, 'intarray')
+        self.assertEqual([2, 3, 4], prop.as_uint32_list())
+        self.assertEqual([2, 3, 4], prop.as_int32_list())
+
+        prop = self.fdt.getprop(self.node, 'int64val')
+        self.assertEqual([0x123456789abcdef0], prop.as_uint64_list())
+        self.assertEqual([0x123456789abcdef0], prop.as_int64_list())
+
+    def test_as_int_lists_negative(self):
+        """Test that the signed accessors differ from the unsigned ones"""
+        self.fdt.resize(self.fdt.totalsize() + 1024)
+        node = self.fdt.path_offset('/spl-test')
+
+        self.fdt.setprop(node, 'negs32', struct.pack('>ll', -1, -2))
+        prop = self.fdt.getprop(node, 'negs32')
+        self.assertEqual([-1, -2], prop.as_int32_list())
+        self.assertEqual([0xffffffff, 0xfffffffe], prop.as_uint32_list())
+
+        self.fdt.setprop(node, 'negs64', struct.pack('>qq', -1, -2))
+        prop = self.fdt.getprop(node, 'negs64')
+        self.assertEqual([-1, -2], prop.as_int64_list())
+        self.assertEqual([0xffffffffffffffff, 0xfffffffffffffffe],
+                         prop.as_uint64_list())
+
+    def test_address_and_size_cells(self):
+        """Test reading the cell counts a node uses for its children"""
+        self.assertEqual(1, self.fdt.address_cells(0))
+        self.assertEqual(1, self.fdt.size_cells(0))
+
+        # i2c@0 overrides both
+        i2c = self.fdt.path_offset('/i2c@0')
+        self.assertEqual(1, self.fdt.address_cells(i2c))
+        self.assertEqual(0, self.fdt.size_cells(i2c))
+
+    def test_mem_rsv(self):
+        """Test adding, reading back and deleting reserve-map records"""
+        self.assertEqual(0, self.fdt.num_mem_rsv())
+
+        # Leave room for the new records
+        self.fdt.resize(self.fdt.totalsize() + 1024)
+        self.fdt.add_mem_rsv(0xdeadbeef00000000, 0x100000)
+        self.fdt.add_mem_rsv(0x1000, 0x2000)
+        self.assertEqual(2, self.fdt.num_mem_rsv())
+        self.assertEqual([0xdeadbeef00000000, 0x100000],
+                         list(self.fdt.get_mem_rsv(0)))
+        self.assertEqual([0x1000, 0x2000], list(self.fdt.get_mem_rsv(1)))
+
+        self.fdt.del_mem_rsv(0)
+        self.assertEqual(1, self.fdt.num_mem_rsv())
+        self.assertEqual([0x1000, 0x2000], list(self.fdt.get_mem_rsv(0)))
+
+    def test_del_mem_rsv_missing(self):
+        """Test deleting a reserve-map record which is not there"""
+        with self.assertRaises(libfdt.FdtException) as exc:
+            self.fdt.del_mem_rsv(0)
+        self.assertEqual(-libfdt.NOTFOUND, exc.exception.err)
+
+    def test_fdtsw_growth(self):
+        """Test that running out of space at least doubles the buffer"""
+        fdtsw = libfdt.FdtSw()
+        size = len(fdtsw._fdt)
+        self.assertEqual(fdtsw.INC_SIZE, size)
+
+        self.assertTrue(fdtsw.check_space(-libfdt.NOSPACE))
+        self.assertEqual(size * 2, len(fdtsw._fdt))
+
+        self.assertTrue(fdtsw.check_space(-libfdt.NOSPACE))
+        self.assertEqual(size * 4, len(fdtsw._fdt))
+
+        # Anything that is not an out-of-space error leaves the buffer alone
+        self.assertFalse(fdtsw.check_space(0))
+        self.assertEqual(size * 4, len(fdtsw._fdt))
+
+    def test_fdtsw_build(self):
+        """Test building a tree large enough to need several expansions"""
+        fdtsw = libfdt.FdtSw()
+        fdtsw.finish_reservemap()
+        fdtsw.begin_node('')
+        for i in range(50):
+            fdtsw.begin_node(f'node{i}')
+            fdtsw.property('data', b'x' * 200)
+            fdtsw.end_node()
+        fdtsw.end_node()
+
+        out = fdtsw.as_fdt()
+        self.assertGreater(out.totalsize(), fdtsw.INC_SIZE * 4)
+        for i in range(50):
+            node = out.path_offset(f'/node{i}')
+            self.assertEqual(f'/node{i}', out.get_path(node))
+            self.assertEqual(b'x' * 200, out.getprop(node, 'data'))
+
+
 class TestFdtUtil(unittest.TestCase):
     """Tests for the fdt_util module
 
@@ -970,7 +1143,7 @@ def run_tests(names, processes):
     test_name = names[0] if names else None
     result = test_util.run_test_suites(
         'test_fdt', False, False, False, False, processes, test_name, None,
-        [TestFdt, TestNode, TestProp, TestFdtUtil])
+        [TestFdt, TestNode, TestProp, TestPylibfdt, TestFdtUtil])
 
     return (0 if result.wasSuccessful() else 1)