Re: [PATCH v2 30/37] arm64: dts: qcom: sm8250: Move PCIe phy and GPIOs to root port node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The commit message claims to move `phy-names` to the root port nodes, but the property is actually deleted and not added.
- [Medium] `wake-gpios` is moved to the root port child node, but it is not permitted by the `qcom,pcie-common.yaml` schema for child nodes.

Pre-existing issues:
- [High] Missing `phy_exit()` on error paths in `qcom_pcie_parse_port()` and `qcom_pcie_parse_legacy_binding()` leading to PHY resource leaks.
--

commit b2d52b29841e8c337d148f97ebcff12f728d5a5f
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

arm64: dts: qcom: sm8250: Move PCIe phy and GPIOs to root port node

The PCIe phy reference and the perst/wake GPIO properties are
per root port and belong in the root port node (pcie@0), not in the
RC controller node.

> Move phys, phy-names, perst-gpios, and wake-gpios
> from the controller to the existing pcieport0 and newly labeled
> pcie1_port0 and pcie2_port0, allowing board-level overrides. Rename

[Severity: Medium]
The commit message explicitly states that phy-names is being moved to the
root port nodes. However, the diff shows it is being deleted from the parent
controller nodes but never added to the destination root port child nodes.

While omitting phy-names might be structurally correct according to the
bindings, does the implementation contradicting the stated intent in the
commit message represent a regression?

> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index eca66d1c1c5b7..a5e2a16f6213e 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -2198,12 +2198,6 @@ pcie0: pcie@1c00000 {
>  
>  			power-domains = <&gcc PCIE_0_GDSC>;
>  
> -			phys = <&pcie0_phy>;
> -			phy-names = "pciephy";
> -
> -			perst-gpios = <&tlmm 79 GPIO_ACTIVE_LOW>;
> -			wake-gpios = <&tlmm 81 GPIO_ACTIVE_LOW>;
> -
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&pcie0_default_state>;
>  			dma-coherent;
> @@ -2218,6 +2212,10 @@ pcieport0: pcie@0 {
>  				#address-cells = <3>;
>  				#size-cells = <2>;
>  				ranges;
> +
> +				phys = <&pcie0_phy>;
> +				reset-gpios = <&tlmm 79 GPIO_ACTIVE_LOW>;
> +				wake-gpios = <&tlmm 81 GPIO_ACTIVE_LOW>;

[Severity: Medium]
Could moving wake-gpios to the pcie@0 child node introduce a schema
validation regression?

In Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml, the
patternProperties for ^pcie@ has unevaluatedProperties: false and only
explicitly permits reg and phys. If wake-gpios is not a standard property
defined in /schemas/pci/pci-pci-bridge.yaml#, might this fail
dt_binding_check?


[Severity: High]
This is a pre-existing issue, but missing phy_exit() on error paths in
qcom_pcie_parse_port() and qcom_pcie_parse_legacy_binding() appears to
lead to PHY resource leaks.

In qcom_pcie_parse_port():

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_parse_port() {
    ...
	ret = phy_init(phy);
	if (ret)
		return ret;

	INIT_LIST_HEAD(&port->perst);

	ret = qcom_pcie_parse_perst(pcie, port, node);
	if (ret)
		return ret;
    ...
}

If qcom_pcie_parse_perst() fails (e.g., returning -EPROBE_DEFER when
failing to acquire the reset GPIO), the function returns the error
immediately without calling phy_exit(phy).

Because the newly allocated port has not yet been added to the pcie->ports
list, the error handling path in qcom_pcie_parse_ports() will not see this
PHY. Does this repeatedly leak clocks and regulators on probe deferral,
preventing power domains from collapsing?

The same pattern exists in qcom_pcie_parse_legacy_binding():

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_parse_legacy_binding() {
    ...
	ret = phy_init(phy);
	if (ret)
		return ret;

	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
	if (!port)
		return -ENOMEM;

	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
	if (!perst)
		return -ENOMEM;
    ...
}

Could these devm_kzalloc() calls also fail and leak the initialized PHY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-wake-v2-0-2744251b1181@oss.qualcomm.com?part=30