linux-kernel - Re: [PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing address translation property

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: <0dd12c70-70f9-1dc1-c5c8-a3ff15be81f6@microchip.com>
Date:   Fri, 12 Aug 2022 08:20:45 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <krzysztof.kozlowski@...aro.org>, <mail@...chuod.ie>,
        <Daire.McNamara@...rochip.com>, <bhelgaas@...gle.com>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <paul.walmsley@...ive.com>, <greentime.hu@...ive.com>,
        <palmer@...belt.com>, <aou@...s.berkeley.edu>,
        <lpieralisi@...nel.org>
CC:     <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing
 address translation property

On 12/08/2022 08:52, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 11/08/2022 23:33, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@...rochip.com>
>>
>> When the PCI controller node was added to the PolarFire SoC dtsi,
>> dt-schema was not able to detect the presence of some undocumented
>> properties due to how it handled unevaluatedProperties. v2022.08
>> introduces better validation, producing the following error:
>>
>> arch/riscv/boot/dts/microchip/mpfs-polarberry.dtb: pcie@...0000000: Unevaluated properties are not allowed ('clock-names', 'microchip,axi-m-atr0' were unexpected)
>>          From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>
>> Fixes: 528a5b1f2556 ("riscv: dts: microchip: add new peripherals to icicle kit device tree")
>> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
>> ---
>> I feel like there's a pretty good chance that this is not the way this
>> should have been done and the property should be marked as deprecated
>> but I don't know enough about PCI to answer that.
> 
> It seems bindings were added incomplete and now based on DTS (which did
> not match bindings), we keep adding "missing" properties. I don't think
> it is good. It creates a precedence where someone might intentionally
> sneak limited bindings (without controversial property) and later claim
> "I forgot to include foo,bar".

Yup, again pretty much the same thoughts as me. I don't think that, even
if the property is valid, should be either named as it is or only work
for translation table 0.

> 
> Therefore the property should pass review just like it is newly added
> property.

SGTM.

> 
>> ---
>>   .../devicetree/bindings/pci/microchip,pcie-host.yaml  | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> index 9b123bcd034c..9ac34b33c4b2 100644
>> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> @@ -71,6 +71,17 @@ properties:
>>     msi-parent:
>>       description: MSI controller the device is capable of using.
>>
>> +  microchip,axi-m-atr0:
> 
> Name is not helping. If it is offset, add such suffix (see
> brcm,iproc-pcie.yaml).
> 
> Unfortunately I don't know PCIe good enough to judge whether the
> property makes any sense or some other ranges-style should be used.

Yup, I think it is similar to that. Except we have 4 tables rather
than one.

> 
>> +    description: |
>> +      Depending on the FPGA bitstream, the AXIM address translation table in the
>> +      PCIe controllers bridge layer may need to be configured. Use this property
>> +      to set the address offset. For more information, see Section 1.3.3,
>> +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
>> +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    minItems: 2
> 
> minItems should not be needed, but you should instead describe the items
> in the matrix.

SGTM. Thanks Krzysztof.

> 
>> +    maxItems: 2
> 
> 
> Best regards,
> Krzysztof