Re: [PATCH v4 09/18] PCI: rcar-gen4: Add Root Port reset support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pci: rcar-gen4: PHY power leak on speed_control() failure
- [Medium] pci: rcar-gen4: memory leak of device_node refcount from of_msi_xlate
--

commit 50f3fefd40d6567632993d0a3d1141361fef7262
Author: Koichiro Den <den@valinux.co.jp>

PCI: rcar-gen4: Add Root Port reset support

Implement the host bridge reset_root_port() callback so PCI error
recovery can reset and reinitialize the R-Car controller. This also
provides the reset operation for the link-down handling added later.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index cbd22e6bb44b9..54260e62293b1 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

[ ... ]

> @@ -661,14 +704,87 @@ static int rcar_gen4_pcie_host_setup(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> +static void rcar_gen4_pcie_quiesce_irqs(struct rcar_gen4_pcie *rcar)
> +{
> +	scoped_guard(raw_spinlock_irqsave, &rcar->app_lock) {
> +		rcar->reinit_pending = true;
> +		rcar_gen4_pcie_app_irq_sync_locked(rcar);
> +	}
> +}
> +
> +static void rcar_gen4_pcie_resume_irqs(struct rcar_gen4_pcie *rcar,
> +				       bool recovered)
> +{
> +	scoped_guard(raw_spinlock_irqsave, &rcar->app_lock) {
> +		rcar->reinit_pending = !recovered;
> +		rcar_gen4_pcie_app_irq_sync_locked(rcar);
> +	}
> +}
> +
> +/*
> + * R-Car Gen4 SoC PCIe controllers and R-Car Gen5 SoC PCIe4 controller have
> + * a single Root Port per instance, so the pci_dev is ignored and the whole
> + * controller is reset. The PCI core serializes the callers through its
> + * device locks.
> + */
> +static int rcar_gen4_pcie_reset_root_port(struct pci_host_bridge *bridge,
> +					  struct pci_dev *pdev)
> +{
> +	struct rcar_gen4_pcie *rcar = dev_get_drvdata(bridge->dev.parent);
> +	struct dw_pcie *dw = &rcar->dw;
> +	struct dw_pcie_rp *pp = &dw->pp;
> +	struct device *dev = dw->dev;
> +	int ret;
> +
> +	/* A failed resume may have left the controller powered down. */
> +	if (dw->suspended)
> +		return -EBUSY;
> +
> +	rcar_gen4_pcie_quiesce_irqs(rcar);
> +
> +	dw_pcie_stop_link(dw);
> +
> +	rcar_gen4_pcie_host_perst_assert(pp, true);
> +
> +	ret = rcar_gen4_pcie_common_reinit(rcar);
> +	if (ret)
> +		goto out;
> +
> +	ret = rcar_gen4_pcie_host_setup(pp);
> +	if (ret)
> +		goto out;

[Severity: Medium]
Does calling rcar_gen4_pcie_host_setup() on every Root Port reset lead to
a device_node refcount leak?

rcar_gen4_pcie_host_setup() calls rcar_gen4_pcie_host_msi_init(), which
calls rcar_gen4_pcie_host_msi_addr(). Inside rcar_gen4_pcie_host_msi_addr():

drivers/pci/controller/dwc/pcie-rcar-gen4.c:rcar_gen4_pcie_host_msi_addr() {
    ...
    of_msi_xlate(dev, &msi_node, 0);
    if (!msi_node)
        return -ENODEV;

    if (!of_device_is_compatible(msi_node, "arm,gic-v3-its"))
        return dev_err_probe(...);
    ...
}

of_msi_xlate() internally acquires a reference to the matched device node,
which requires the caller to release it using of_node_put(). Since
rcar_gen4_pcie_host_msi_addr() does not appear to release this reference,
won't this leak a kobject reference dynamically on every reset event?

> +
> +	ret = dw_pcie_setup_rc(pp);
> +	if (ret)
> +		goto out;
> +
> +	ret = dw_pcie_start_link(dw);
> +	if (ret)
> +		goto out;

[Severity: Medium]
If dw_pcie_start_link() fails, can this leave the PHY permanently powered
on?

It looks like rcar_gen4_pcie_start_link() powers on the PHY via
ltssm_control(..., true) before calling speed_control(). For Gen5 devices,
rcar_gen5_pcie_speed_control() can fail. If this occurs, the error is
returned without calling ltssm_control(..., false).

When dw_pcie_start_link() returns an error here, the code jumps to out:
which skips dw_pcie_stop_link(), leaving the PHY powered on. Is there
a missing power off in this error path?

> +
> +	/* Ignore errors, the link may come up later */
> +	dw_pcie_wait_for_link(dw);
> +	dev_dbg(dev, "Root Port reset completed\n");
> +
> +out:
> +	rcar_gen4_pcie_resume_irqs(rcar, !ret);
> +	if (ret)
> +		dev_err(dev, "Root Port reset failed (%d); interrupts stay masked until a reset succeeds\n",
> +			ret);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015033103.3076869-1-den@valinux.co.jp?part=9