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

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The commit message inaccurately describes the changes made to the `phy-names` and `wake-gpios` properties.

Pre-existing issues:
- [High] Missing `phy_exit()` in `qcom_pcie_parse_port()` error path leads to PHY initialization count leak on probe deferral.
--

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

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

This patch moves the PCIe phys, perst-gpios, and wake-gpios properties from
the PCIe root complex controller node to the root port node (pcie@0) for
pcie0 and pcie1. It also renames perst-gpios to reset-gpios.

> 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 pcie0_port0 and pcie1_port0, adding labels to

[Severity: Low]
The commit message states phy-names and wake-gpios are moved to both
pcie0_port0 and pcie1_port0.

However, in the diff, phy-names is removed entirely rather than moved, and
wake-gpios is only added to pcie0_port0 since it wasn't originally present in
pcie1.

Should the commit message be updated to accurately reflect the changes made?

> these nodes to allow board-level overrides, and renaming perst-gpios
> to reset-gpios to match the binding used in the root port context.
>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chaitanya.chundru@oss.qualcomm.com>

> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index 8da494de4308a..f13c67e93db30 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi

[ ... ]

> @@ -1920,6 +1914,10 @@ pcie@0 {
>  				#address-cells = <3>;
>  				#size-cells = <2>;
>  				ranges;
> +
> +				phys = <&pcie0_phy>;
> +				reset-gpios = <&tlmm 35 GPIO_ACTIVE_HIGH>;
> +				wake-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>;
>  			};
>  		};
>  

[Severity: High]
This is a pre-existing issue, but moving the phys property to the pcie@0 root
port node causes the sm8150 device tree to now utilize the
qcom_pcie_parse_port() code path in the pcie-qcom driver.

Does this expose a resource leak on probe deferral?

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() returns -EPROBE_DEFER, will this code leak the
PHY initialization count because phy_exit(phy) is not called before
returning?

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