| Message ID | 20220823145649.3118479-10-robh@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dt-bindings: remoteproc: Add missing (unevaluated|additional)Properties on child nodes | expand |
On 23/08/2022 17:56, Rob Herring wrote: > In order to ensure only documented properties are present, node schemas > must have unevaluatedProperties or additionalProperties set to false > (typically). > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 1 + > .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 1 + > .../devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > index e76c861165dd..e4a7da8020f4 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > @@ -140,6 +140,7 @@ properties: > > glink-edge: > $ref: qcom,glink-edge.yaml# > + unevaluatedProperties: false Is it actually needed? The qcom,glink-edge.yaml has additionalProperties:false, so I expect it to complain if anything appears here. Best regards, Krzysztof
On Thu, Aug 25, 2022 at 3:23 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/08/2022 17:56, Rob Herring wrote: > > In order to ensure only documented properties are present, node schemas > > must have unevaluatedProperties or additionalProperties set to false > > (typically). > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 1 + > > .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 1 + > > .../devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml | 1 + > > 3 files changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > > index e76c861165dd..e4a7da8020f4 100644 > > --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > > @@ -140,6 +140,7 @@ properties: > > > > glink-edge: > > $ref: qcom,glink-edge.yaml# > > + unevaluatedProperties: false > > Is it actually needed? The qcom,glink-edge.yaml has > additionalProperties:false, so I expect it to complain if anything > appears here. Perhaps not, but I'm trying to come up with a meta-schema to check these though I'm not sure I can get to no warnings which is how I found all these cases. The main remaining warnings are bus child node pattern schemas which can perhaps be handled with 'additionalProperties: true'. The rule I have says if properties or patternProperties is present then unevaluatedProperties or additionalProperties must be. To handle this case, I think we'd have to walk the $ref and check it. Anyways, we can hold off on this one until when and if there's a meta-schema in place. Rob
On 25/08/2022 16:13, Rob Herring wrote: > On Thu, Aug 25, 2022 at 3:23 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 23/08/2022 17:56, Rob Herring wrote: >>> In order to ensure only documented properties are present, node schemas >>> must have unevaluatedProperties or additionalProperties set to false >>> (typically). >>> >>> Signed-off-by: Rob Herring <robh@kernel.org> >>> --- >>> .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 1 + >>> .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 1 + >>> .../devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml | 1 + >>> 3 files changed, 3 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>> index e76c861165dd..e4a7da8020f4 100644 >>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>> @@ -140,6 +140,7 @@ properties: >>> >>> glink-edge: >>> $ref: qcom,glink-edge.yaml# >>> + unevaluatedProperties: false >> >> Is it actually needed? The qcom,glink-edge.yaml has >> additionalProperties:false, so I expect it to complain if anything >> appears here. > > Perhaps not, but I'm trying to come up with a meta-schema to check > these though I'm not sure I can get to no warnings which is how I > found all these cases. The main remaining warnings are bus child node > pattern schemas which can perhaps be handled with > 'additionalProperties: true'. The rule I have says if properties or > patternProperties is present then unevaluatedProperties or > additionalProperties must be. To handle this case, I think we'd have > to walk the $ref and check it. > > Anyways, we can hold off on this one until when and if there's a > meta-schema in place. For me adding unevaluatedProp:false everywhere with $ref is okay and it makes the code easier to read - no need to dive into referenced schema to remember if it allows or does not allow additional properties. It is also a safer choice if referenced schema forgot to set additionalProp:false. However if referenced schema has additionalProp:false, then unevaluatedProp:false here is redundant and question is whether the redundancy is worth additional readability/obviousness. To me, unevaluatedProp:false here would during review save time - no need to jump into referenced schema to check what is there. If we make it a rule / coding convention, then I am in. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Tue, 23 Aug 2022 09:56:41 -0500, Rob Herring wrote: > In order to ensure only documented properties are present, node schemas > must have unevaluatedProperties or additionalProperties set to false > (typically). > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 1 + > .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 1 + > .../devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml | 1 + > 3 files changed, 3 insertions(+) > Applied, thanks!
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml index e76c861165dd..e4a7da8020f4 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml @@ -140,6 +140,7 @@ properties: glink-edge: $ref: qcom,glink-edge.yaml# + unevaluatedProperties: false description: Qualcomm G-Link subnode which represents communication edge, channels and devices related to the DSP. diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml index da1a5de3d38b..b4de0521a89d 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml @@ -154,6 +154,7 @@ properties: glink-edge: $ref: qcom,glink-edge.yaml# + unevaluatedProperties: false description: Qualcomm G-Link subnode which represents communication edge, channels and devices related to the DSP. diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml index 3f06d66cbe47..b6bd33438584 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml @@ -107,6 +107,7 @@ properties: glink-edge: $ref: qcom,glink-edge.yaml# + unevaluatedProperties: false description: Qualcomm G-Link subnode which represents communication edge, channels and devices related to the ADSP.
In order to ensure only documented properties are present, node schemas must have unevaluatedProperties or additionalProperties set to false (typically). Signed-off-by: Rob Herring <robh@kernel.org> --- .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 1 + .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 1 + .../devicetree/bindings/remoteproc/qcom,sc7280-wpss-pil.yaml | 1 + 3 files changed, 3 insertions(+)