linux-kernel - Re: [PATCH 17/43] dt-bindings: phy: qcom,qmp-pcie: add missing child node schema

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Jul 2022 14:11:28 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Koul <vkoul@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 17/43] dt-bindings: phy: qcom,qmp-pcie: add missing child
 node schema

On Tue, Jul 05, 2022 at 01:56:32PM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2022 13:51, Johan Hovold wrote:
> > On Tue, Jul 05, 2022 at 12:18:37PM +0200, Krzysztof Kozlowski wrote:
> >> On 05/07/2022 11:42, Johan Hovold wrote:
> >>> Add the missing the description of the PHY-provider child node which was
> >>> ignored when converting to DT schema.
> >>>
> >>> Also fix up the incorrect description that claimed that one child node
> >>> per lane was required.
> >>>
> >>> Fixes: ccf51c1cedfd ("dt-bindings: phy: qcom,qmp: Convert QMP PHY bindings to yaml")
> >>> Signed-off-by: Johan Hovold <johan+linaro@...nel.org>
> >>> ---
> >>>  .../bindings/phy/qcom,qmp-pcie-phy.yaml       | 88 ++++++++++++++++++-
> >>>  1 file changed, 85 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> >>> index ff1577f68a00..5a1ebf874559 100644
> >>> --- a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> >>> +++ b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> >>> @@ -69,9 +69,37 @@ properties:
> > 
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - qcom,sm8250-qmp-gen3x2-pcie-phy
> >>> +              - qcom,sm8250-qmp-modem-pcie-phy
> >>> +              - qcom,sm8450-qmp-gen4x2-pcie-phy
> >>> +    then:
> >>> +      patternProperties:
> >>> +        "^phy@[0-9a-f]+$":
> >>> +          properties:
> >>> +            reg:
> >>> +              items:
> >>> +                - description: TX lane 1
> >>> +                - description: RX lane 1
> >>> +                - description: PCS
> >>> +                - description: TX lane 2
> >>> +                - description: RX lane 2
> >>> +                - description: PCS_MISC
> >>> +    else:
> >>> +      patternProperties:
> >>> +        "^phy@[0-9a-f]+$":
> >>> +          properties:
> >>> +            reg:
> >>> +              minItems: 3
> >>> +              maxItems: 4
> >>> +              items:
> >>> +                - description: TX
> >>> +                - description: RX
> >>> +                - description: PCS
> >>> +                - description: PCS_MISC
> >>> +      if:
> >>
> >> Do not include if within other if. Just split the entire section to its
> >> own if:.
> > 
> > That sounds like it would just obfuscate the logic. The else clause
> > specified 3-4 registers and the nested if determines which compatibles
> > use which by further narrowing the range.
> > 
> > If you move it out to the else: this would be really hard understand and
> > verify.
> 
> Every bindings are expected to do that way and most of them are doing
> it: define broad constraints in properties:, then define strict
> constraints per each variant. Easy to follow code. This binding is not
> particularly special to make it different than other ones. Doing
> semi-strict constraints in if: and then additional constrain in nested
> if: is not easy to understand and verify.

Ok, so you want to flatten this by repeating also the register
descriptions?

That wouldn't hurt readability as much, but doing so would be more error
prone as it's easy to miss adding a new compatible in every group of
conditionals and there's no else clause to catch the mistake.

Right know the logic is

	if dual-lane
		items = 6
	else
		items = 3 or 4
		if single-lane-exception
			items = 3
		else
			items = 4

Flattening this gives

	if dual-lane
		items = 6
	if single-lane-normal
		items = 4
	if single-lane-exception
		items = 3

Which means that every compatible must now be listed in one of the
conditionals.

Fine with me, but please confirm that I understood you correctly.

Johan