Re: [PATCH] PCI: qcom: Handle mixed PERST#/PHY DT configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Probing fails for legacy device trees due to a GPIO request conflict (-EBUSY).
- [High] Unconditional recursion in `qcom_pcie_parse_perst()` duplicates the `pcie->reset` GPIO descriptor for every descendant node.
- [High] Missing `phy_exit()` cleanup on error path in `qcom_pcie_parse_port()` causes hardware state and resource leak.
--

commit 1a0ea25971de08bb67bf6aea3574504602a6d020
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

PCI: qcom: Handle mixed PERST#/PHY DT configuration

The driver currently supports PERST# and PHY DT configuration on either the
RC or RP node. This commit adds support for mixed setups where PHY is on
the RP node but PERST# remains on the RC node by reusing the RC PERST#
GPIO and emitting a warning.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 9fdfc88ac1512..8235961d692f2 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -1703,6 +1704,11 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
>  	struct gpio_desc *reset;
>  	int ret;
>  
> +	if (pcie->reset) {
> +		reset = pcie->reset;
> +		goto skip_perst_parsing;
> +	}
> +
>  	if (!of_find_property(np, "reset-gpios", NULL))
>  		goto parse_child_node;
>  
> @@ -1721,6 +1727,7 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
>  		return PTR_ERR(reset);
>  	}
>  
> +skip_perst_parsing:
>  	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);

When pcie->reset is configured, this jumps to skip_perst_parsing, adds
pcie->reset to the port's list, and then falls through to parse_child_node:

parse_child_node:
	for_each_available_child_of_node_scoped(np, child) {
		ret = qcom_pcie_parse_perst(pcie, port, child);
		if (ret)
			return ret;
	}

Since pcie->reset remains set globally, does this recursively call itself
for all descendant nodes, skip property parsing again, and append the exact
same GPIO descriptor to the port's list for every descendant?

Will this redundantly toggle the same GPIO multiple times and unintentionally
ignore any endpoint-specific reset-gpios properties defined in child nodes?

[ ... ]

> @@ -1778,6 +1785,14 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	struct device *dev = pcie->pci->dev;
>  	int ret = -ENODEV;
>  
> +	if (of_find_property(dev->of_node, "perst-gpios", NULL)) {
> +		pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +		if (IS_ERR(pcie->reset))
> +			return PTR_ERR(pcie->reset);
> +
> +		dev_warn(dev, "Reusing PERST# from Root Complex node. DT needs to be fixed!\n");
> +	}
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {

If there are no child "pci" nodes, this function returns -ENODEV. Does the
caller then fall back to qcom_pcie_parse_legacy_binding() which will also
attempt to request the exact same "perst" GPIO?

Since devm_gpiod_get_optional() persists for the lifetime of the probe and
GPIOLIB enforces exclusive access by default, will the second request fail
with -EBUSY and break the probe path for legacy platforms?

>  		if (!of_node_is_type(of_port, "pci"))
>  			continue;
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)
>  			goto err_port_del;
>  	}

This issue wasn't introduced by this patch, but if qcom_pcie_parse_perst()
returns an error (such as -EPROBE_DEFER or -ENOMEM) from within
qcom_pcie_parse_port(), is phy_init() successfully called before the error
is encountered?

If qcom_pcie_parse_port() returns early without calling phy_exit(), and the
failing port has not yet been added to the pcie->ports list, does the
err_port_del cleanup loop skip this port?

err_port_del:
	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
		...
		phy_exit(port->phy);

Could this leave the PHY active and unbalance regulator/clock enable counts
across probe retries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-mix_perst_phy_dts-v1-1-9eff6ee9b51a@oss.qualcomm.com?part=1