| Message ID | 20261004011851.866833-1-marek.vasut+renesas@mailbox.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] PCI: rcar-gen4: Add missing PM ops | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] pcie: deadlocks due to ioremap and request_firmware in NOIRQ PM phase -- commit e6c055f1eedf6bd5c13e231da24f0c6a76a24ba2 Author: Marek Vasut <marek.vasut+renesas@mailbox.org> PCI: rcar-gen4: Add missing PM ops The R-Car Gen4 PCIe controller needs to gracefully suspend and resume its link across system suspend/resume cycles to support power domains shutting down. This patch adds suspend_noirq and resume_noirq PM operations to tear down the link on suspend and restart it on resume. > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > index 2e7a8d85c0060..fceecbd16355c 100644 > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c [ ... ] > @@ -1062,6 +1064,26 @@ static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable > return 0; > } > > +static int rcar_gen4_pcie_suspend_noirq(struct device *dev) > +{ > + struct rcar_gen4_pcie *rcar = dev_get_drvdata(dev); > + > + if (rcar->drvdata->mode != DW_PCIE_RC_TYPE) > + return 0; > + > + return dw_pcie_suspend_noirq(&rcar->dw); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this code trigger a deadlock or kernel panic during the NOIRQ PM phase? Setting pp->use_atu_msg to true causes dw_pcie_suspend_noirq() to eventually call ioremap(): dw_pcie_suspend_noirq() dw_pcie_pme_turn_off() ioremap() Since this runs during the suspend_noirq phase with local interrupts disabled and the system frozen, ioremap() allocates memory with GFP_KERNEL and may sleep, which is strictly prohibited in NOIRQ context. > +} > + > +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; > + > + return dw_pcie_resume_noirq(&rcar->dw); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Will this result in a sleeping function called from invalid context warning? During the resume_noirq phase, this calls into the firmware request API: dw_pcie_resume_noirq() dw_pcie_start_link() rcar_gen4_pcie_ltssm_control() rcar_gen4_pcie_download_phy_firmware() request_firmware() Similar to the suspend path, request_firmware() allocates memory using GFP_KERNEL and may block. Executing this with interrupts disabled in the NOIRQ phase is unsafe and can lead to a system panic. > +} > + > static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c index 2e7a8d85c0060..fceecbd16355c 100644 --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c @@ -671,6 +671,8 @@ static int rcar_gen4_add_dw_pcie_rp(struct rcar_gen4_pcie *rcar) return -ENODEV; pp->num_vectors = MAX_MSI_IRQS; + /* Reserve an iATU window for the generic PME_Turn_Off implementation. */ + pp->use_atu_msg = true; pp->ops = &rcar_gen4_pcie_host_ops; return dw_pcie_host_init(pp); @@ -1062,6 +1064,26 @@ static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable return 0; } +static int rcar_gen4_pcie_suspend_noirq(struct device *dev) +{ + struct rcar_gen4_pcie *rcar = dev_get_drvdata(dev); + + if (rcar->drvdata->mode != DW_PCIE_RC_TYPE) + return 0; + + return dw_pcie_suspend_noirq(&rcar->dw); +} + +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; + + return dw_pcie_resume_noirq(&rcar->dw); +} + static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = { .init = rcar_gen4_pcie_common_init, .deinit = rcar_gen4_pcie_common_deinit, @@ -1127,10 +1149,15 @@ static const struct of_device_id rcar_gen4_pcie_of_match[] = { }; MODULE_DEVICE_TABLE(of, rcar_gen4_pcie_of_match); +static DEFINE_NOIRQ_DEV_PM_OPS(rcar_gen4_pcie_pm_ops, + rcar_gen4_pcie_suspend_noirq, + rcar_gen4_pcie_resume_noirq); + static struct platform_driver rcar_gen4_pcie_driver = { .driver = { .name = "pcie-rcar-gen4", .of_match_table = rcar_gen4_pcie_of_match, + .pm = pm_sleep_ptr(&rcar_gen4_pcie_pm_ops), .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, .probe = rcar_gen4_pcie_probe,
The R-Car Gen4 PCIe controller is part of a power domain. On R-Car S4 and V4H, this is an always-on power domain which is not shut down in suspend. On R-Car V4M, this is a dedicated A2PCIPHY power domain, which is shut down during suspend, and the controller loses state, which prevents the PCIe from working after resume. Fix this by adding generic suspend/resume noirq ops for the controller, which tear the link down on suspend, and restart it on resume. Use the same PM ops on all of R-Car Gen4 to gracefully suspend and resume the PCIe link on V4H and S4 too. Test case which demonstrates the problem on R-Car V4M: " $ hexdump -vC /sys/bus/pci/devices/0000:00:00.0/config > /tmp/pre $ echo s2idle > /sys/power/mem_sleep $ echo platform > /sys/power/pm_test $ echo mem > /sys/power/state $ hexdump -vC /sys/bus/pci/devices/0000:00:00.0/config > /tmp/post $ diff -Naru /tmp/pre /tmp/post ... -00000000 12 19 32 00 07 05 10 00 00 00 04 06 00 00 01 00 +00000000 12 19 32 00 07 05 10 00 00 00 00 ff 00 00 80 00 ^^^^^ Class 0604->00ff " Fixes: 0d0c551011df ("PCI: rcar-gen4: Add R-Car Gen4 PCIe controller support for host mode") Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Tested-by: Koichiro Den <den@valinux.co.jp> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> --- Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Conor Dooley <conor+dt@kernel.org> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Koichiro Den <den@valinux.co.jp> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> Cc: Magnus Damm <magnus.damm@gmail.com> Cc: Manivannan Sadhasivam <mani@kernel.org> Cc: Rob Herring <robh@kernel.org> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-pci@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org --- V2: - Add TB from Geert for RC mode - Use pm_sleep_ptr() - Call HOST suspend/resume ops only in case the controller is in RC mode V3: - Add RB from Geert - Make rcar_gen4_pcie_pm_ops() static - Pull the mode from driver data V4: - Add TB from Den-san V5: - Set pp->use_atu_msg = true to use generic MSG TLP for sending PME_Turn_Off (as suggested by Den-san) --- drivers/pci/controller/dwc/pcie-rcar-gen4.c | 27 +++++++++++++++++++++ 1 file changed, 27 insertions(+)