edtlib: fix last modified semantic in included property specs

scripts/dts/python-devicetree/src/devicetree/edtlib.py

@@ -163,7 +163,9 @@ class Binding:
 
     def __init__(self, path: Optional[str], fname2path: Dict[str, str],
                  raw: Any = None, require_compatible: bool = True,
-                 require_description: bool = True):
+                 require_description: bool = True,
+                 inc_allowlist: Optional[List[str]] = None,
+                 inc_blocklist: Optional[List[str]] = None):
         """
         Binding constructor.
 
@@ -191,16 +193,36 @@ def __init__(self, path: Optional[str], fname2path: Dict[str, str],
           "description:" line. If False, a missing "description:" is
           not an error. Either way, "description:" must be a string
           if it is present in the binding.
+
+        inc_allowlist:
+          The property-allowlist filter set by including bindings.
+
+        inc_blocklist:
+          The property-blocklist filter set by including bindings.
         """
         self.path: Optional[str] = path
         self._fname2path: Dict[str, str] = fname2path
 
+        self._inc_allowlist: Optional[List[str]] = inc_allowlist
+        self._inc_blocklist: Optional[List[str]] = inc_blocklist
+
         if raw is None:
             if path is None:
                 _err("you must provide either a 'path' or a 'raw' argument")
             with open(path, encoding="utf-8") as f:
                 raw = yaml.load(f, Loader=_BindingLoader)
 
+        # Get the properties this binding modifies
+        # before we merge the included ones.
+        last_modified_props = list(raw.get("properties", {}).keys())
+
+        # Map property names to their specifications:
+        # - first, _merge_includes() will recursively populate prop2specs with
+        #   the properties specified by the included bindings
+        # - eventually, we'll update prop2specs with the properties
+        #   this binding itself defines or modifies
+        self.prop2specs: Dict[str, 'PropertySpec'] = {}
+
         # Merge any included files into self.raw. This also pulls in
         # inherited child binding definitions, so it has to be done
         # before initializing those.
@@ -224,10 +246,11 @@ def __init__(self, path: Optional[str], fname2path: Dict[str, str],
         # Make sure this is a well defined object.
         self._check(require_compatible, require_description)
 
-        # Initialize look up tables.
-        self.prop2specs: Dict[str, 'PropertySpec'] = {}
-        for prop_name in self.raw.get("properties", {}).keys():
+        # Update specs with the properties this binding defines or modifies.
+        for prop_name in last_modified_props:
             self.prop2specs[prop_name] = PropertySpec(prop_name, self)
+
+        # Initialize look up tables.
         self.specifier2cells: Dict[str, List[str]] = {}
         for key, val in self.raw.items():
             if key.endswith("-cells"):
@@ -291,18 +314,41 @@ def _merge_includes(self, raw: dict, binding_path: Optional[str]) -> dict:
 
         if isinstance(include, str):
             # Simple scalar string case
-            _merge_props(merged, self._load_raw(include), None, binding_path,
-                         False)
+            # Load YAML file and register property specs into prop2specs.
+            inc_raw = self._load_raw(include, self._inc_allowlist,
+                                     self._inc_blocklist)
+
+            _merge_props(merged, inc_raw, None, binding_path,  False)
         elif isinstance(include, list):
             # List of strings and maps. These types may be intermixed.
             for elem in include:
                 if isinstance(elem, str):
-                    _merge_props(merged, self._load_raw(elem), None,
-                                 binding_path, False)
+                    # Load YAML file and register property specs into prop2specs.
+                    inc_raw = self._load_raw(elem, self._inc_allowlist,
+                                             self._inc_blocklist)
+
+                    _merge_props(merged, inc_raw, None, binding_path, False)
                 elif isinstance(elem, dict):
                     name = elem.pop('name', None)
+
+                    # Merge this include property-allowlist filter
+                    # with filters from including bindings.
                     allowlist = elem.pop('property-allowlist', None)
+                    if allowlist is not None:
+                        if self._inc_allowlist:
+                            allowlist.extend(self._inc_allowlist)
+                    else:
+                        allowlist = self._inc_allowlist
+
+                    # Merge this include property-blocklist filter
+                    # with filters from including bindings.
                     blocklist = elem.pop('property-blocklist', None)
+                    if blocklist is not None:
+                        if self._inc_blocklist:
+                            blocklist.extend(self._inc_blocklist)
+                    else:
+                        blocklist = self._inc_blocklist
+
                     child_filter = elem.pop('child-binding', None)
 
                     if elem:
@@ -313,10 +359,12 @@ def _merge_includes(self, raw: dict, binding_path: Optional[str]) -> dict:
                     _check_include_dict(name, allowlist, blocklist,
                                         child_filter, binding_path)
 
-                    contents = self._load_raw(name)
+                    # Load YAML file, and register (filtered) property specs
+                    # into prop2specs.
+                    contents = self._load_raw(name,
+                                              allowlist, blocklist,
+                                              child_filter)
 
-                    _filter_properties(contents, allowlist, blocklist,
-                                       child_filter, binding_path)
                     _merge_props(merged, contents, None, binding_path, False)
                 else:
                     _err(f"all elements in 'include:' in {binding_path} "
@@ -336,11 +384,17 @@ def _merge_includes(self, raw: dict, binding_path: Optional[str]) -> dict:
 
         return raw
 
-    def _load_raw(self, fname: str) -> dict:
+
+    def _load_raw(self, fname: str,
+                  allowlist: Optional[List[str]] = None,
+                  blocklist: Optional[List[str]] = None,
+                  child_filter: Optional[dict] = None) -> dict:
         # Returns the contents of the binding given by 'fname' after merging
-        # any bindings it lists in 'include:' into it. 'fname' is just the
-        # basename of the file, so we check that there aren't multiple
-        # candidates.
+        # any bindings it lists in 'include:' into it, according to the given
+        # property filters.
+        #
+        # Will also register the (filtered) included property specs
+        # into prop2specs.
 
         path = self._fname2path.get(fname)
 
@@ -352,8 +406,55 @@ def _load_raw(self, fname: str) -> dict:
             if not isinstance(contents, dict):
                 _err(f'{path}: invalid contents, expected a mapping')
 
+        # Apply constraints to included YAML contents.
+        _filter_properties(contents,
+                           allowlist, blocklist,
+                           child_filter, self.path)
+
+        # Register included property specs.
+        self._add_included_prop2specs(fname, contents, allowlist, blocklist)
+
         return self._merge_includes(contents, path)
 
+    def _add_included_prop2specs(self, fname: str, contents: dict,
+                                 allowlist: Optional[List[str]] = None,
+                                 blocklist: Optional[List[str]] = None) -> None:
+        # Registers the properties specified by an included binding file
+        # into the properties this binding supports/requires (aka prop2specs).
+        #
+        # Consider "this" binding B includes I1 which itself includes I2.
+        #
+        # We assume to be called in that order:
+        # 1) _add_included_prop2spec(B, I1)
+        # 2) _add_included_prop2spec(B, I2)
+        #
+        # Where we don't want I2 "taking ownership" for properties
+        # modified by I1.
+        #
+        # So we:
+        # - first create a binding that represents the included file
+        # - then add the property specs defined by this binding to prop2specs,
+        #   without overriding the specs modified by an including binding
+        #
+        # Note: Unfortunately, we can't cache these base bindings,
+        # as a same YAML file may be included with different filters
+        # (property-allowlist and such), leading to different contents.
+
+        inc_binding = Binding(
+            self._fname2path[fname],
+            self._fname2path,
+            contents,
+            require_compatible=False,
+            require_description=False,
+            # Recursively pass filters to included bindings.
+            inc_allowlist=allowlist,
+            inc_blocklist=blocklist,
+        )
+
+        for prop, spec in inc_binding.prop2specs.items():
+            if prop not in self.prop2specs:
+                self.prop2specs[prop] = spec
+
     def _check(self, require_compatible: bool, require_description: bool):
         # Does sanity checking on the binding.
 

scripts/dts/python-devicetree/tests/test-bindings-include/base.yaml

@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+properties:
+  x:
+    type: int
+  y:
+    type: int

scripts/dts/python-devicetree/tests/test-bindings-include/inc-base.yaml

@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+include: base.yaml

scripts/dts/python-devicetree/tests/test-bindings-include/modified.yaml

@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+include: base.yaml
+
+properties:
+  x:
+    required: true

scripts/dts/python-devicetree/tests/test-bindings-include/top-allows.yaml

@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+description: Test property-allowlist filters set by including bindings
+
+compatible: "top-allowlist"
+
+include:
+  - name: inc-base.yaml
+    property-allowlist:
+      - x

scripts/dts/python-devicetree/tests/test-bindings-include/top-blocks.yaml

@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+description: Test property-blocklist filters set by including bindings.
+
+compatible: "top-blocklist"
+
+include:
+  - name: inc-base.yaml
+    property-blocklist:
+      - x

scripts/dts/python-devicetree/tests/test-bindings-include/top.yaml

@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+description: |
+  Top-level binding file for testing included property spec paths.
+
+  base.yaml: specifies properties "x" and "y"
+  modified.yaml: includes base.yaml, modifies property "x"
+  top.yaml (this file): includes modified.yaml, specifies property "p"
+
+  From the top-level binding, we expect:
+  - "x" was last modified in modified.yaml
+  - "y" was last modified in base.yaml
+  - "p" was last modified in top.yaml
+
+compatible: top-level
+
+include: modified.yaml
+
+properties:
+  p:
+    type: int

scripts/dts/python-devicetree/tests/test_edtlib.py

@@ -365,6 +365,35 @@ def test_include_filters():
         assert set(child.prop2specs.keys()) == {'child-prop-1', 'child-prop-2',
                                                 'x', 'z'}  # root level 'y' is blocked
 
+def test_include_paths():
+    '''Test "last modified" semantic for included bindings paths.'''
+
+    fname2path = {'base.yaml': 'test-bindings-include/base.yaml',
+                  'modified.yaml': 'test-bindings-include/modified.yaml'}
+
+    with from_here():
+        top = edtlib.Binding('test-bindings-include/top.yaml', fname2path)
+
+        assert 'modified.yaml' == os.path.basename(top.prop2specs["x"].path)
+        assert 'base.yaml' == os.path.basename(top.prop2specs["y"].path)
+        assert 'top.yaml' == os.path.basename(top.prop2specs["p"].path)
+
+def test_include_filters_included_bindings():
+    '''Test filters set by including bindings.'''
+    fname2path = {'base.yaml': 'test-bindings-include/base.yaml',
+                  'inc-base.yaml': 'test-bindings-include/inc-base.yaml'}
+
+    with from_here():
+        top_allows = edtlib.Binding('test-bindings-include/top-allows.yaml', fname2path)
+    assert top_allows.prop2specs.get("x")
+    assert not top_allows.prop2specs.get("y")
+
+    with from_here():
+        top_blocks = edtlib.Binding('test-bindings-include/top-blocks.yaml', fname2path)
+    assert not top_blocks.prop2specs.get("x")
+    assert top_blocks.prop2specs.get("y")
+
+
 
 def test_bus():
     '''Test 'bus:' and 'on-bus:' in bindings'''