Re: [PATCH v2 31/37] arm64: dts: qcom: sm8350: 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:
- [Medium] The commit message explicitly claims to move `phy-names` to the port nodes, but the property was completely removed.

Pre-existing issues:
- [High] qcom_pcie_phy_power_on() blindly calls qcom_pcie_phy_power_off() on error, leading to unbalanced regulator disables.
--

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

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

This commit moves the PCIe PHY references and perst/wake GPIO properties
from the root complex controller nodes to their respective root port nodes.
It also renames the perst-gpios property to reset-gpios to conform with
the root port bindings.

> Move phys and phy-names from the controller to pcie0_port0 and pcie1_port0, adding labels to these nodes to allow board-level overrides.

[Severity: Medium]
It appears "phy-names" was only deleted from the parent controller nodes and
never added back to the port nodes.

Was the intention to drop "phy-names" entirely, or should it be added to
pcie0_port0 and pcie1_port0?

> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> index c830953156ec6..4515a1ccb930b 100644
> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> @@ -1583,12 +1583,9 @@ pcie0: pcie@1c00000 {
>  
>  			power-domains = <&gcc PCIE_0_GDSC>;
>  
> -			phys = <&pcie0_phy>;
> -			phy-names = "pciephy";
> -
>  			status = "disabled";
>  
> -			pcie@0 {
> +			pcie0_port0: pcie@0 {
>  				device_type = "pci";
>  				reg = <0x0 0x0 0x0 0x0 0x0>;
>  				bus-range = <0x01 0xff>;
> @@ -1596,6 +1593,8 @@ pcie@0 {
>  				#address-cells = <3>;
>  				#size-cells = <2>;
>  				ranges;
> +
> +				phys = <&pcie0_phy>;

[Severity: Medium]
Should phy-names = "pciephy" be added here to match the commit message?

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential flaw in the driver's
error handling path that could lead to unbalanced regulator disables.

If phy_power_on() fails in drivers/pci/controller/dwc/pcie-qcom.c, the error
handling blindly calls qcom_pcie_phy_power_off():

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_phy_power_on() {
    ...
    if (ret)
        qcom_pcie_phy_power_off(pcie);
    ...
}

This unconditionally iterates over all parsed ports to call phy_power_off():

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_phy_power_off() {
    ...
    list_for_each_entry(port, &pcie->ports, list)
        phy_power_off(port->phy);
    ...
}

This includes the port that just failed to power on (where power_count is 0)
as well as any subsequent ports in the list that haven't been attempted.

Calling phy_power_off() on an unpowered PHY decrements its power_count to -1
and unconditionally calls phy_pm_runtime_put() and regulator_disable().

Could this trigger a warning in the core regulator framework and inadvertently
cut power to other devices sharing the regulator?

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