Re: [PATCH v2 13/16] dt-bindings: i2c: tegra-bpmp: Convert to json-schema

[Rob Herring <robh@kernel.org>]

On Thu, Dec 2, 2021 at 3:08 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Dec 2, 2021 at 11:55 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Wed, Dec 01, 2021 at 12:42:07PM -0600, Rob Herring wrote:
> > > On Wed, Dec 1, 2021 at 11:42 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >

[...]

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