linux-kernel - Re: [PATCH 4/4] dt-bindings: net: dsa: mediatek,mt7530: update json-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]

Message-ID: <24e251d7-f5db-5715-463d-333f5dfbfceb@arinc9.com>
Date:   Fri, 12 Aug 2022 17:06:57 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Sander Vanheule <sander@...nheule.net>,
        René van Dorst <opensource@...rst.com>,
        Daniel Golle <daniel@...rotopia.org>, erkin.bozoglu@...ont.com,
        Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] dt-bindings: net: dsa: mediatek,mt7530: update
 json-schema



On 12.08.2022 16:48, Krzysztof Kozlowski wrote:
> On 12/08/2022 16:41, Arınç ÜNAL wrote:
>> On 12.08.2022 10:01, Krzysztof Kozlowski wrote:
>>> On 12/08/2022 09:57, Krzysztof Kozlowski wrote:
>>>> On 12/08/2022 01:09, Arınç ÜNAL wrote:
>>>>>>> -patternProperties:
>>>>>>> -  "^(ethernet-)?ports$":
>>>>>>> -    type: object
>>>>>>
>>>>>> Actually four patches...
>>>>>>
>>>>>> I don't find this change explained in commit msg. What is more, it looks
>>>>>> incorrect. All properties and patternProperties should be explained in
>>>>>> top-level part.
>>>>>>
>>>>>> Defining such properties (with big piece of YAML) in each if:then: is no
>>>>>> readable.
>>>>>
>>>>> I can't figure out another way. I need to require certain properties for
>>>>> a compatible string AND certain enum/const for certain properties which
>>>>> are inside patternProperties for "^(ethernet-)?port@[0-9]+$" by reading
>>>>> the compatible string.
>>>>
>>>> requiring properties is not equal to defining them and nothing stops you
>>>> from defining all properties top-level and requiring them in
>>>> allOf:if:then:patternProperties.
>>>>
>>>>
>>>>> If I put allOf:if:then under patternProperties, I can't do the latter.
>>>>
>>>> You can.
>>
>> Am I supposed to do something like this:
>>
>> patternProperties:
>>     "^(ethernet-)?ports$":
>>       type: object
>>
>>       patternProperties:
>>         "^(ethernet-)?port@[0-9]+$":
>>           type: object
>>           description: Ethernet switch ports
>>
>>           unevaluatedProperties: false
>>
>>           properties:
>>             reg:
>>               description:
>>                 Port address described must be 5 or 6 for CPU port and
>>                 from 0 to 5 for user ports.
>>
>>           allOf:
>>             - $ref: dsa-port.yaml#
>>             - if:
>>                 properties:
>>                   label:
>>                     items:
>>                       - const: cpu
>>               then:
>>                 allOf:
>>                   - if:
>>                       properties:
> 
> Not really, this is absolutely unreadable.
> 
> Usually the way it is handled is:
> 
> patternProperties:
>     "^(ethernet-)?ports$":
>       type: object
> 
>       patternProperties:
>         "^(ethernet-)?port@[0-9]+$":
>           type: object
>           description: Ethernet switch ports
>           unevaluatedProperties: false
>           ... regular stuff follows
> 
> allOf:
>   - if:
>       properties:
>         compatible:
>           .....
>     then:
>       patternProperties:
>         "^(ethernet-)?ports$":
>           patternProperties:
>             "^(ethernet-)?port@[0-9]+$":
>               properties:
>                 reg:
>                   const: 5
> 
> 
> I admit that it is still difficult to parse, which could justify
> splitting to separate schema. Anyway the point of my comment was to
> define all properties in top level, not in allOf.
> 
> allOf should be used to constrain these properties.

The problem is:
- only specific values of reg are allowed if label is cpu.
- only specific values of phy-mode are allowed if reg is 5 or 6.

This forces me to define properties under allOf:if:then. Splitting to 
separate schema (per compatible string?) wouldn't help in this case.

I can split patternProperties to two sections, but I can't directly 
define the reg property like you put above.

I can at least split mediatek,mt7531 to a separate schema to have less 
patternProperties on a single binding.

What do you think?

Arınç