Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

[Brian Norris <briannorris@chromium.org>]

Hi,

On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> Since the Qcom platforms rely on pwrctrl framework to control the power
> supplies, allow it to control PERST# also. PERST# should be toggled during
> the power-on and power-off scenarios.
> 
> But the controller driver still need to assert PERST# during the controller
> initialization. So only skip the deassert if pwrctrl usage is detected. The
> pwrctrl framework will deassert PERST# after turning on the supplies.
> 
> The usage of pwrctrl framework is detected based on the new DT binding
> i.e., with the presence of PERST# and PHY properties in the Root Port node
> instead of the host bridge node.
> 
> When the legacy binding is used, PERST# is only controlled by the
> controller driver since it is not reliable to detect whether pwrctrl is
> used or not. So the legacy platforms are untouched by this commit.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

> @@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>  	if (ret)
>  		return ret;
>  
> +	devfn = of_pci_get_devfn(node);
> +	if (devfn < 0)
> +		return -ENOENT;
> +
> +	pp->perst[PCI_SLOT(devfn)] = reset;

It seems like you assume a well-written device tree, such that this
PCI_SLOT(devfn) doesn't overflow the perst[] array. It seems like we
should guard against that somehow.

Also see my comment below, where I believe even a well-written device
tree could trip this up.

> +
>  	port->reset = reset;
>  	port->phy = phy;
>  	INIT_LIST_HEAD(&port->list);
> @@ -1734,10 +1746,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>  
>  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  {
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
>  	struct device *dev = pcie->pci->dev;
>  	struct qcom_pcie_port *port, *tmp;
> +	int child_cnt;
>  	int ret = -ENOENT;
>  
> +	child_cnt = of_get_available_child_count(dev->of_node);

I think you're assuming "available children" correlate precisely with a
0-indexed array of ports. But what if, e.g., port 0 is disabled in the
device tree, and only port 1 is available? Then you'll overflow.

> +	if (!child_cnt)
> +		return ret;
> +
> +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);

IIUC, you kfree() this on error, but otherwise, you never free it. I
also see that this driver can't actually be unbound (commit f9a666008338
("PCI: qcom: Make explicitly non-modular")), so technically there's no
way to "leak" this other than by probe errors...
...but it still seems like devm_*() would fit better.

(NB: I'm not sure I agree with commit f9a666008338 that "[driver unbind]
doesn't have a sensible use case anyway". That just sounds like
laziness. And it *can* have a useful purpose for testing.)

Brian

> +	if (!pp->perst)
> +		return -ENOMEM;
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)