On Thu, Dec 2, 2021 at 3:08 PM Rob Herring [off-list ref] wrote:

On Thu, Dec 2, 2021 at 11:55 AM Thierry Reding [off-list ref] wrote:

quoted

On Wed, Dec 01, 2021 at 12:42:07PM -0600, Rob Herring wrote:

quoted

On Wed, Dec 1, 2021 at 11:42 AM Thierry Reding [off-list ref] wrote:

quoted

[...]

quoted

quoted

quoted

However, a side-effect seems to be that now it also ignores any
properties that aren't defined anywhere. So for example if I touch
up the example in firmware/nvidia,tegra186-bpmp.yaml and add a bogus
"foo-bar = <0>;" property in the BPMP I2C node, then it'll blindly
accept that as valid.

Do you have unevaluatedProperties within the i2c node? It only applies
to 1 level, and you can't have a parent+child schema evaluated with
another child (or parent+child) schema. This is why the graph schema
is done the way it is and why we're splitting spi-controller.yaml
child node schema out to spi-peripheral.yaml.

Let me give an example based on a schema that's already upstream. So
looking at this:

        Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml

it does include spi-controller.yaml via an allOf: [ $ref: ... ], so it
uses unevaluatedProperties to validate against any generic SPI
controller properties. For example, #address-cells and #size-cells are
validated based on the schema from spi-controller.yaml.

However, if I now apply the following patch to add an undocumented
property to the example, then validation doesn't fail as I would expect
it to.

Indeed you are right. The problem is 'additionalProperties: true' in
spi-controller.yaml makes everything evaluated. I thought
'additionalProperties: true' was equivalent to the default, but that's
not how it's working. Now to figure out if this is correct operation
or not. No wonder there were relatively few fixes when
'unevaluatedProperties' got implemented...

Based on the json-schema test cases[1], it looks like the tool is
doing the right thing and our schemas are wrong.

It's a quick fixup in the tools though to just delete any
'additionalProperties: true':

index 3cc5e428b0eb..a0f22aab935a 100644

--- a/dtschema/lib.py
+++ b/dtschema/lib.py

@@ -367,6 +367,9 @@ def fixup_sub_schema(schema, is_prop):
     if not isinstance(schema, dict):
         return

+    if 'additionalProperties' in schema and

schema['additionalProperties'] == True:
+        schema.pop('additionalProperties', None)
+
     schema.pop('description', None)
     fixup_interrupts(schema)
     if is_prop:

I'm leaning towards this route besides being easy because the whole
reason for the explicit 'additionalProperties: true' was so that
missing 'additionalProperties' is flagged as an error. The only other
way I've come up with handling this is making common schema use a
different meta-schema.

The result with the above is about 150 warnings. I quickly got this
down to 100 with only a couple of fixes, but the rest appear to be
individual schemas...

Rob

[1] https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft2019-09/unevaluatedProperties.json#L230