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@xxxxxxxxxx>
> ---
> 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)
