linux-kernel - Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: Enforce check for PHY, PERST# properties

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] [day] [month] [year] [list]

Message-ID: <20251108-toad-of-hypothetical-opportunity-ebfa74@kuoka>
Date: Sat, 8 Nov 2025 12:59:50 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Abraham I <kishon@...nel.org>, Bjorn Andersson <andersson@...nel.org>, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: Enforce check for PHY,
 PERST# properties

On Thu, Nov 06, 2025 at 04:57:16PM +0530, Manivannan Sadhasivam wrote:
> Currently, the binding supports specifying the required PHY, PERST#
> properties in two ways:
> 
> 1. Controller node (deprecated)
> 	- phys
> 	- perst-gpios
> 
> 2. Root Port node
> 	- phys
> 	- reset-gpios
> 
> But there is no check to make sure that the both variants are not mixed.
> For instance, if the Controller node specifies 'phys', 'reset-gpios',

Schema already does not allow it, unless I missed which schema defines
reset-gpios in controller node.

> or if the Root Port node specifies 'phys', 'perst-gpios', then the driver
> will fail as reported. Hence, enforce the check in the binding to catch
> these issues.

I do not see such check.

> 
> It is also possible that DTs could have 'phys' property in Controller node
> and 'reset-gpios' properties in the Root Port node. It will also be a
> problem, but it is not possible to catch these cross-node issues in the
> binding.

... so this commit changes nothing?

The commit actually does change, but something completely different than
you write here, so entire commit msg is describing entirely different
cast. What you achieve here is to require perst-gpios, if controller
node defined phys. Unfortunately your commit msg does not explain why
perst-gpios are now required...

> 
> Reported-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> Reported-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>

That's too many tags. Either someone reported you bug or someone
suggested you to do something, not both (and proposing solution is not
suggesting a commit since you already knew you need to make the commit
because of bug...)


> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-common.yaml        | 16 ++++++++++++++++
>  .../devicetree/bindings/pci/qcom,pcie-sc8180x.yaml       |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> index ab2509ec1c4b40ac91a93033d1bab1b12c39362f..d56c0dc2ae4d3944294ca50cab708915c9f60ea8 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> @@ -111,6 +111,14 @@ patternProperties:
>        phys:
>          maxItems: 1
>  
> +    oneOf:
> +      - required:
> +          - phys
> +          - reset-gpios
> +      - properties:
> +          phys: false
> +          reset-gpios: false
> +
>      unevaluatedProperties: false
>  
>  required:
> @@ -129,6 +137,14 @@ anyOf:
>    - required:
>        - msi-map
>  
> +oneOf:
> +  - required:
> +      - phys
> +      - perst-gpios
> +  - properties:
> +      phys: false
> +      perst-gpios: false
> +
>  allOf:
>    - $ref: /schemas/pci/pci-host-bridge.yaml#
>  
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
> index 34a4d7b2c8459aeb615736f54c1971014adb205f..17abc7f7b7e9d71777380ddbfe90288e6187a827 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
> @@ -77,6 +77,7 @@ unevaluatedProperties: false
>  examples:
>    - |
>      #include <dt-bindings/clock/qcom,gcc-sc8180x.h>
> +    #include <dt-bindings/gpio/gpio.h>
>      #include <dt-bindings/interconnect/qcom,sc8180x.h>
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
> @@ -164,5 +165,7 @@ examples:
>  
>              resets = <&gcc GCC_PCIE_0_BCR>;
>              reset-names = "pci";
> +
> +            perst-gpios = <&tlmm 175 GPIO_ACTIVE_LOW>;
>          };
>      };
> 
> -- 
> 2.48.1
>