Re: [PATCH v3] PCI: rcar-gen4: Add missing PM ops

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] PCIe host controller fails to re-initialize on R-Car V4M after resume if any endpoint does not support D3cold.
- [High] Endpoint mode (EP) does not recover from suspend on R-Car V4M due to explicitly skipping PM state restoration.

Pre-existing issues:
- [High] The `msi_node` device node obtained from `of_msi_xlate()` is leaked because its refcount is never decremented.
--

commit 924c8a29b2859d29f5190b0088001c235ae190fa
Author: Marek Vasut [off-list ref]

PCI: rcar-gen4: Add missing PM ops

Adds generic suspend/resume noirq ops to tear down and restart the PCIe link
during suspend/resume for R-Car Gen4. This supports power domains that shut
down in suspend, such as on R-Car V4M.

[Severity: High]
Does returning 0 here cause the endpoint (EP) mode to not recover from
suspend on R-Car V4M?

If the V4M A2PCIPHY power domain shuts down, removing power from the EP
controller, explicitly skipping PM state restoration will leave the EP
hardware dead after system resume.

+
+	return dw_pcie_suspend_noirq(&rcar->dw);

[Severity: High]
Can the host controller fail to re-initialize on system resume if an attached
endpoint does not support D3cold?

If an attached endpoint does not support D3cold, the generic helper
dw_pcie_suspend_noirq() returns 0 early:

drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_suspend_noirq() {
    ...
    if (!pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable))
        return 0;
    ...
}

Because it returns 0 without setting pci->suspended = true, the subsequent
resume will skip initialization:

drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_resume_noirq() {
    ...
    if (!pci->suspended)
        return 0;
    ...
}

Since the A2PCIPHY power domain is unconditionally shut down by the generic
PM core, the hardware loses state. Does skipping re-initialization here leave
the PCIe bus permanently unusable?

+}
+
+static int rcar_gen4_pcie_resume_noirq(struct device *dev)
+{
+	struct rcar_gen4_pcie *rcar = dev_get_drvdata(dev);
+
+	if (rcar->drvdata->mode != DW_PCIE_RC_TYPE)
+		return 0;

[Severity: High]
Does this explicit bypass also prevent the EP controller from being
reconfigured after power loss, as mentioned above?

+
+	return dw_pcie_resume_noirq(&rcar->dw);

[Severity: High]
This isn't a bug introduced by this patch, but does calling the generic resume
helper introduce a recurring device_node leak?

On every resume, dw_pcie_resume_noirq() will call pci->pp.ops->init(), which
invokes rcar_gen4_pcie_host_msi_addr(). Inside that function, of_msi_xlate()
acquires a reference to the msi_node:

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;
    ...
}

The function returns without ever calling of_node_put(msi_node). Because this
patch now causes init to run on every suspend/resume cycle instead of just at
probe time, does this leak the msi_node reference on every system resume?

+}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907163554.725127-1-marek.vasut+renesas@mailbox.org?part=1