* [PATCH v6 1/3] PCI: host-common: Fix early bus-walk exit in d3cold_possible()
2026-09-09 12:24 [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Krishna Chaitanya Chundru
@ 2026-09-09 12:24 ` Krishna Chaitanya Chundru
2026-09-09 12:29 ` sashiko-bot
2026-09-09 12:24 ` [PATCH v6 2/3] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check Krishna Chaitanya Chundru
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-09-09 12:24 UTC (permalink / raw)
To: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: konrad.dybcio, linux-pci, linux-kernel, linux-arm-msm,
Krishna Chaitanya Chundru
__pci_host_common_d3cold_possible() returns -EOPNOTSUPP for the first
downstream device it finds outside PCI_D3hot, and pci_walk_bus() aborts
the walk as soon as its callback returns nonzero. Any device enumerated
after the disqualifying one -- including a wakeup-enabled, PME-from-D3cold
capable endpoint -- is then never visited, so pme_capable can come back
false even though such a device exists on the bus.
Since pci_host_common_d3cold_possible() already returns false whenever
any device disqualifies D3cold, aborting the walk buys nothing for the
plain suspend path: the overall bool result is unaffected. But it
silently drops pme_capable detection for any device ordered after the
disqualifying one.
This matters for the upcoming shutdown path in particular: unlike plain
suspend, shutdown forces the link into L2/D3cold regardless of whether
pci_host_common_d3cold_possible() itself allows it (see the following
"force_d3cold" changes), so at shutdown time it's common for an
endpoint to still be in D0 and disqualify D3cold while a later,
PME-capable device is never visited. dw_pcie_suspend_noirq() still
uses "pme_capable" to set pci->pp.skip_pwrctrl_off, so an inaccurate
result here can cause Vaux/wakeup support to be dropped for a device
that actually supports PME from D3cold.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/pci/controller/pci-host-common.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index a23907a875e5..c2c852c64aa8 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -275,21 +275,16 @@ static int __pci_host_common_d3cold_possible(struct pci_dev *pdev,
return 0;
if (pdev->current_state != PCI_D3hot)
- goto exit;
+ *flags &= ~PCI_HOST_D3COLD_ALLOWED;
if (device_may_wakeup(&pdev->dev)) {
- if (!pci_pme_capable(pdev, PCI_D3cold))
- goto exit;
- else
+ if (pci_pme_capable(pdev, PCI_D3cold))
*flags |= PCI_HOST_PME_D3COLD_CAPABLE;
+ else
+ *flags &= ~PCI_HOST_D3COLD_ALLOWED;
}
return 0;
-
-exit:
- *flags &= ~PCI_HOST_D3COLD_ALLOWED;
-
- return -EOPNOTSUPP;
}
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 1/3] PCI: host-common: Fix early bus-walk exit in d3cold_possible()
2026-09-09 12:24 ` [PATCH v6 1/3] PCI: host-common: Fix early bus-walk exit in d3cold_possible() Krishna Chaitanya Chundru
@ 2026-09-09 12:29 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 12:29 UTC (permalink / raw)
To: Krishna Chaitanya Chundru; +Cc: linux-pci
> __pci_host_common_d3cold_possible() returns -EOPNOTSUPP for the first
> downstream device it finds outside PCI_D3hot, and pci_walk_bus() aborts
> the walk as soon as its callback returns nonzero. Any device enumerated
> after the disqualifying one -- including a wakeup-enabled, PME-from-D3cold
> capable endpoint -- is then never visited, so pme_capable can come back
> false even though such a device exists on the bus.
>
> Since pci_host_common_d3cold_possible() already returns false whenever
> any device disqualifies D3cold, aborting the walk buys nothing for the
> plain suspend path: the overall bool result is unaffected. But it
> silently drops pme_capable detection for any device ordered after the
> disqualifying one.
>
> This matters for the upcoming shutdown path in particular: unlike plain
> suspend, shutdown forces the link into L2/D3cold regardless of whether
> pci_host_common_d3cold_possible() itself allows it (see the following
> "force_d3cold" changes), so at shutdown time it's common for an
> endpoint to still be in D0 and disqualify D3cold while a later,
> PME-capable device is never visited. dw_pcie_suspend_noirq() still
> uses "pme_capable" to set pci->pp.skip_pwrctrl_off, so an inaccurate
> result here can cause Vaux/wakeup support to be dropped for a device
> that actually supports PME from D3cold.
>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-shutdown-v6-0-62b7bd123236@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 2/3] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check
2026-09-09 12:24 [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Krishna Chaitanya Chundru
2026-09-09 12:24 ` [PATCH v6 1/3] PCI: host-common: Fix early bus-walk exit in d3cold_possible() Krishna Chaitanya Chundru
@ 2026-09-09 12:24 ` Krishna Chaitanya Chundru
2026-09-09 12:34 ` sashiko-bot
2026-09-09 12:24 ` [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback Krishna Chaitanya Chundru
2026-09-09 12:38 ` [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Konrad Dybcio
3 siblings, 1 reply; 13+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-09-09 12:24 UTC (permalink / raw)
To: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: konrad.dybcio, linux-pci, linux-kernel, linux-arm-msm,
Krishna Chaitanya Chundru
dw_pcie_suspend_noirq() normally calls pci_host_common_d3cold_possible()
to check whether every downstream endpoint can be put into D3cold before
bothering to move the link to L2. If no endpoint supports it, the
function returns early and leaves the link up.
Querying D3cold support during shutdown is actively harmful, not just
slow: pci_host_common_d3cold_possible() requires every active endpoint
to already be in PCI_D3hot, and returns false otherwise. If any endpoint
is still in D0 -- which is common, since endpoint drivers aren't
guaranteed to have suspended by the time the host's shutdown path runs
the check fails and dw_pcie_suspend_noirq() returns early without ever
moving the link to L2, leaving it up right up to the point where the
system cuts power/clocks to the controller.
Add a force parameter to dw_pcie_suspend_noirq() that callers set
explicitly to force this behavior, still call
pci_host_common_d3cold_possible() unconditionally, since it's also how
pme_capable gets set, but ignore its return value and force L2 entry
regardless when force is set.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 2 +-
drivers/pci/controller/dwc/pci-layerscape.c | 2 +-
drivers/pci/controller/dwc/pcie-designware-host.c | 7 ++++---
drivers/pci/controller/dwc/pcie-designware.h | 4 ++--
drivers/pci/controller/dwc/pcie-eswin.c | 2 +-
drivers/pci/controller/dwc/pcie-nxp-s32g.c | 2 +-
drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
drivers/pci/controller/dwc/pcie-stm32.c | 2 +-
drivers/pci/controller/dwc/pcie-ultrarisc.c | 2 +-
9 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 4105c28d180c..63799b1c2013 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1738,7 +1738,7 @@ static int imx_pcie_suspend_noirq(struct device *dev)
imx_pcie_assert_perst(imx_pcie, true);
imx_pcie->drvdata->enable_ref_clk(imx_pcie, false);
} else {
- return dw_pcie_suspend_noirq(imx_pcie->pci);
+ return dw_pcie_suspend_noirq(imx_pcie->pci, false);
}
return 0;
diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
index 14d6ac4fc53f..aca1b0acb4ed 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -382,7 +382,7 @@ static int ls_pcie_suspend_noirq(struct device *dev)
if (!pcie->drvdata->pm_support)
return 0;
- return dw_pcie_suspend_noirq(pcie->pci);
+ return dw_pcie_suspend_noirq(pcie->pci, false);
}
static int ls_pcie_resume_noirq(struct device *dev)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f5a38e6fd8d7..21830c05d147 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -1222,16 +1222,17 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
return 0;
}
-int dw_pcie_suspend_noirq(struct dw_pcie *pci)
+int dw_pcie_suspend_noirq(struct dw_pcie *pci, bool force)
{
- bool pme_capable = false;
+ bool d3cold_possible, pme_capable = false;
int ret = 0;
u32 val;
if (!dw_pcie_link_up(pci))
goto stop_link;
- if (!pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable))
+ d3cold_possible = pci_host_common_d3cold_possible(pci->pp.bridge, &pme_capable);
+ if (!d3cold_possible && !force)
return 0;
if (pci->pp.ops->pme_turn_off) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index a53ac27cd244..cec1c64c3a5e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -860,7 +860,7 @@ static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
const char *dw_pcie_ltssm_status_string(enum dw_pcie_ltssm ltssm);
#ifdef CONFIG_PCIE_DW_HOST
-int dw_pcie_suspend_noirq(struct dw_pcie *pci);
+int dw_pcie_suspend_noirq(struct dw_pcie *pci, bool force);
int dw_pcie_resume_noirq(struct dw_pcie *pci);
void dw_handle_msi_irq(struct dw_pcie_rp *pp);
void dw_pcie_msi_init(struct dw_pcie_rp *pp);
@@ -873,7 +873,7 @@ int dw_pcie_allocate_domains(struct dw_pcie_rp *pp);
void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
int where);
#else
-static inline int dw_pcie_suspend_noirq(struct dw_pcie *pci)
+static inline int dw_pcie_suspend_noirq(struct dw_pcie *pci, bool force)
{
return 0;
}
diff --git a/drivers/pci/controller/dwc/pcie-eswin.c b/drivers/pci/controller/dwc/pcie-eswin.c
index ce8d64f8a395..06fc488f80c7 100644
--- a/drivers/pci/controller/dwc/pcie-eswin.c
+++ b/drivers/pci/controller/dwc/pcie-eswin.c
@@ -367,7 +367,7 @@ static int eswin_pcie_suspend_noirq(struct device *dev)
{
struct eswin_pcie *pcie = dev_get_drvdata(dev);
- return dw_pcie_suspend_noirq(&pcie->pci);
+ return dw_pcie_suspend_noirq(&pcie->pci, false);
}
static int eswin_pcie_resume_noirq(struct device *dev)
diff --git a/drivers/pci/controller/dwc/pcie-nxp-s32g.c b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
index 31e1169b8ab6..43174a06c60a 100644
--- a/drivers/pci/controller/dwc/pcie-nxp-s32g.c
+++ b/drivers/pci/controller/dwc/pcie-nxp-s32g.c
@@ -365,7 +365,7 @@ static int s32g_pcie_suspend_noirq(struct device *dev)
struct s32g_pcie *s32g_pp = dev_get_drvdata(dev);
struct dw_pcie *pci = &s32g_pp->pci;
- return dw_pcie_suspend_noirq(pci);
+ return dw_pcie_suspend_noirq(pci, false);
}
static int s32g_pcie_resume_noirq(struct device *dev)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ee63a6ec99de..2693a98d0b27 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -2421,7 +2421,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
if (!pcie)
return 0;
- ret = dw_pcie_suspend_noirq(pcie->pci);
+ ret = dw_pcie_suspend_noirq(pcie->pci, false);
if (ret)
return ret;
diff --git a/drivers/pci/controller/dwc/pcie-stm32.c b/drivers/pci/controller/dwc/pcie-stm32.c
index 349618ea5b9c..7b3a8c1b04a4 100644
--- a/drivers/pci/controller/dwc/pcie-stm32.c
+++ b/drivers/pci/controller/dwc/pcie-stm32.c
@@ -78,7 +78,7 @@ static int stm32_pcie_suspend_noirq(struct device *dev)
struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
int ret;
- ret = dw_pcie_suspend_noirq(&stm32_pcie->pci);
+ ret = dw_pcie_suspend_noirq(&stm32_pcie->pci, false);
if (ret)
return ret;
diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
index 51e89180490d..cb2781a1557f 100644
--- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
+++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
@@ -187,7 +187,7 @@ static int ultrarisc_pcie_suspend_noirq(struct device *dev)
if (pci->suspended)
return 0;
- ret = dw_pcie_suspend_noirq(pci);
+ ret = dw_pcie_suspend_noirq(pci, false);
if (ret)
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/3] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check
2026-09-09 12:24 ` [PATCH v6 2/3] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check Krishna Chaitanya Chundru
@ 2026-09-09 12:34 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 12:34 UTC (permalink / raw)
To: Krishna Chaitanya Chundru; +Cc: linux-pci
> dw_pcie_suspend_noirq() normally calls pci_host_common_d3cold_possible()
> to check whether every downstream endpoint can be put into D3cold before
> bothering to move the link to L2. If no endpoint supports it, the
> function returns early and leaves the link up.
>
> Querying D3cold support during shutdown is actively harmful, not just
> slow: pci_host_common_d3cold_possible() requires every active endpoint
> to already be in PCI_D3hot, and returns false otherwise. If any endpoint
> is still in D0 -- which is common, since endpoint drivers aren't
> guaranteed to have suspended by the time the host's shutdown path runs
> the check fails and dw_pcie_suspend_noirq() returns early without ever
> moving the link to L2, leaving it up right up to the point where the
> system cuts power/clocks to the controller.
>
> Add a force parameter to dw_pcie_suspend_noirq() that callers set
> explicitly to force this behavior, still call
> pci_host_common_d3cold_possible() unconditionally, since it's also how
> pme_capable gets set, but ignore its return value and force L2 entry
> regardless when force is set.
>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-shutdown-v6-0-62b7bd123236@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback
2026-09-09 12:24 [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Krishna Chaitanya Chundru
2026-09-09 12:24 ` [PATCH v6 1/3] PCI: host-common: Fix early bus-walk exit in d3cold_possible() Krishna Chaitanya Chundru
2026-09-09 12:24 ` [PATCH v6 2/3] PCI: dwc: Force L2 link entry on shutdown/reboot without D3cold check Krishna Chaitanya Chundru
@ 2026-09-09 12:24 ` Krishna Chaitanya Chundru
2026-09-09 12:34 ` sashiko-bot
2026-09-09 12:36 ` Konrad Dybcio
2026-09-09 12:38 ` [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Konrad Dybcio
3 siblings, 2 replies; 13+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-09-09 12:24 UTC (permalink / raw)
To: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: konrad.dybcio, linux-pci, linux-kernel, linux-arm-msm,
Krishna Chaitanya Chundru, Manivannan Sadhasivam
From: Manivannan Sadhasivam <mani@kernel.org>
PCIe host controllers should bring the link down cleanly before system
shutdown/reboot proceeds to remove power/clocks from the controller.
Without this, the link may still be up and endpoints still have
transactions in flight when power/clocks are cut, which can trip SMMU
translation faults or NoC protocol errors.
Reuse dw_pcie_suspend_noirq() in the shutdown path to force the link
into L2, putting it into D3cold.
device_shutdown() runs with interrupts enabled, unlike suspend_noirq().
Mask the chained MSI IRQ(s) and the Global IRQ before tearing down the
link and clocks/PHY, since a late/spurious interrupt could otherwise
reach a handler that touches now-unclocked PARF/DBI registers.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/pci/controller/dwc/pcie-qcom.c | 35 ++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 2693a98d0b27..3133e2b2eda6 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1988,6 +1988,9 @@ static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
pp->use_imsi_rx = true;
dw_pcie_msi_init(pp);
+ /* Stash pci so qcom_pcie_shutdown() can mask the MSI IRQ(s) later */
+ platform_set_drvdata(to_platform_device(dev), pci);
+
return devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
}
@@ -2408,6 +2411,37 @@ static int qcom_pcie_probe(struct platform_device *pdev)
return ret;
}
+static void qcom_pcie_shutdown(struct platform_device *pdev)
+{
+ const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
+ struct qcom_pcie *pcie;
+ struct dw_pcie *pci;
+
+ if (pcie_cfg && pcie_cfg->firmware_managed) {
+ /*
+ * Firmware owns the link teardown and clock/PHY shutdown in
+ * this mode; Linux only owns the chained MSI IRQ(s), which
+ * still need to be masked off before shutdown proceeds.
+ */
+ pci = platform_get_drvdata(pdev);
+ } else {
+ pcie = platform_get_drvdata(pdev);
+ pci = pcie->pci;
+
+ if (pcie->global_irq)
+ disable_irq(pcie->global_irq);
+ }
+
+ if (pci->pp.use_imsi_rx)
+ dw_pcie_free_msi(&pci->pp);
+
+ if (pcie)
+ dw_pcie_suspend_noirq(pcie->pci, true);
+
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+}
+
static int qcom_pcie_suspend_noirq(struct device *dev)
{
struct qcom_pcie *pcie;
@@ -2598,5 +2632,6 @@ static struct platform_driver qcom_pcie_driver = {
.pm = &qcom_pcie_pm_ops,
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
},
+ .shutdown = qcom_pcie_shutdown,
};
builtin_platform_driver(qcom_pcie_driver);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback
2026-09-09 12:24 ` [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback Krishna Chaitanya Chundru
@ 2026-09-09 12:34 ` sashiko-bot
2026-09-09 12:36 ` Konrad Dybcio
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 12:34 UTC (permalink / raw)
To: Krishna Chaitanya Chundru; +Cc: linux-pci
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The local variable `pcie` is used uninitialized on the `firmware_managed` code path, leading to undefined behavior and a likely kernel panic when evaluated and dereferenced.
- [High] Calling `dw_pcie_free_msi()` during `.shutdown` destroys the active MSI IRQ domain while child endpoints still have IRQs mapped, triggering a spurious kernel warning and leaving dangling state.
--
commit 27c88a203aab0fef576a235de5067454639a8757
Author: Manivannan Sadhasivam <mani@kernel.org>
PCI: qcom: Implement shutdown() callback
This commit implements a shutdown callback for PCIe host controllers to
bring the link down cleanly before system shutdown or reboot proceeds.
It reuses dw_pcie_suspend_noirq() in the shutdown path to force the link
into L2 and masks chained MSI IRQs to prevent late spurious interrupts.
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 2693a98d0b279..3133e2b2eda69 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[ ... ]
> @@ -2408,6 +2411,37 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
> + const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
> + struct qcom_pcie *pcie;
> + struct dw_pcie *pci;
> +
> + if (pcie_cfg && pcie_cfg->firmware_managed) {
> + /*
> + * Firmware owns the link teardown and clock/PHY shutdown in
> + * this mode; Linux only owns the chained MSI IRQ(s), which
> + * still need to be masked off before shutdown proceeds.
> + */
> + pci = platform_get_drvdata(pdev);
> + } else {
> + pcie = platform_get_drvdata(pdev);
> + pci = pcie->pci;
> +
> + if (pcie->global_irq)
> + disable_irq(pcie->global_irq);
> + }
> +
> + if (pci->pp.use_imsi_rx)
> + dw_pcie_free_msi(&pci->pp);
[Severity: High]
Does calling dw_pcie_free_msi() here trigger a core IRQ subsystem warning?
During system shutdown, child endpoints have their .shutdown callbacks
executed rather than .remove, so they do not naturally release their
allocated MSI vectors.
dw_pcie_free_msi() unconditionally destroys the IRQ domain:
drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_free_msi() {
...
irq_domain_remove(pp->irq_domain);
}
Will destroying this domain while child endpoints still hold IRQ mappings
leave dangling IRQ descriptors and trigger a warning when checking if the
domain tree is empty?
> +
> + if (pcie)
> + dw_pcie_suspend_noirq(pcie->pci, true);
[Severity: Critical]
Is the pcie variable left uninitialized in the firmware_managed path?
When pcie_cfg->firmware_managed is true, pci is initialized but pcie is
not. It looks like this branch will then evaluate stack garbage for the
condition check.
If it evaluates to true, could this cause a kernel panic by dereferencing
stack garbage when accessing pcie->pci in dw_pcie_suspend_noirq()?
> +
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-shutdown-v6-0-62b7bd123236@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback
2026-09-09 12:24 ` [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback Krishna Chaitanya Chundru
2026-09-09 12:34 ` sashiko-bot
@ 2026-09-09 12:36 ` Konrad Dybcio
2026-09-09 12:40 ` Krishna Chaitanya Chundru
1 sibling, 1 reply; 13+ messages in thread
From: Konrad Dybcio @ 2026-09-09 12:36 UTC (permalink / raw)
To: Krishna Chaitanya Chundru, Jingoo Han, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas
Cc: linux-pci, linux-kernel, linux-arm-msm
On 9/9/26 2:24 PM, Krishna Chaitanya Chundru wrote:
> From: Manivannan Sadhasivam <mani@kernel.org>
>
> PCIe host controllers should bring the link down cleanly before system
> shutdown/reboot proceeds to remove power/clocks from the controller.
> Without this, the link may still be up and endpoints still have
> transactions in flight when power/clocks are cut, which can trip SMMU
> translation faults or NoC protocol errors.
>
> Reuse dw_pcie_suspend_noirq() in the shutdown path to force the link
> into L2, putting it into D3cold.
>
> device_shutdown() runs with interrupts enabled, unlike suspend_noirq().
> Mask the chained MSI IRQ(s) and the Global IRQ before tearing down the
> link and clocks/PHY, since a late/spurious interrupt could otherwise
> reach a handler that touches now-unclocked PARF/DBI registers.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
[...]
> +static void qcom_pcie_shutdown(struct platform_device *pdev)
> +{
> + const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
> + struct qcom_pcie *pcie;
> + struct dw_pcie *pci;
> +
> + if (pcie_cfg && pcie_cfg->firmware_managed) {
> + /*
> + * Firmware owns the link teardown and clock/PHY shutdown in
> + * this mode; Linux only owns the chained MSI IRQ(s), which
> + * still need to be masked off before shutdown proceeds.
> + */
> + pci = platform_get_drvdata(pdev);
> + } else {
> + pcie = platform_get_drvdata(pdev);
This is no less than an intriguing choice.. please store the same
element in both cases so we don't have to do this
> + pci = pcie->pci;
> +
> + if (pcie->global_irq)
> + disable_irq(pcie->global_irq);
This changed the ordering of disabling things versus the previous
revision, although I don't think it should matter given this
IRQ currently only services the linkdown event
Konrad
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback
2026-09-09 12:36 ` Konrad Dybcio
@ 2026-09-09 12:40 ` Krishna Chaitanya Chundru
2026-09-09 13:18 ` Konrad Dybcio
0 siblings, 1 reply; 13+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-09-09 12:40 UTC (permalink / raw)
To: Konrad Dybcio, Jingoo Han, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas
Cc: linux-pci, linux-kernel, linux-arm-msm
On 9/9/2026 6:06 PM, Konrad Dybcio wrote:
> On 9/9/26 2:24 PM, Krishna Chaitanya Chundru wrote:
>> From: Manivannan Sadhasivam <mani@kernel.org>
>>
>> PCIe host controllers should bring the link down cleanly before system
>> shutdown/reboot proceeds to remove power/clocks from the controller.
>> Without this, the link may still be up and endpoints still have
>> transactions in flight when power/clocks are cut, which can trip SMMU
>> translation faults or NoC protocol errors.
>>
>> Reuse dw_pcie_suspend_noirq() in the shutdown path to force the link
>> into L2, putting it into D3cold.
>>
>> device_shutdown() runs with interrupts enabled, unlike suspend_noirq().
>> Mask the chained MSI IRQ(s) and the Global IRQ before tearing down the
>> link and clocks/PHY, since a late/spurious interrupt could otherwise
>> reach a handler that touches now-unclocked PARF/DBI registers.
>>
>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
> [...]
>
>> +static void qcom_pcie_shutdown(struct platform_device *pdev)
>> +{
>> + const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
>> + struct qcom_pcie *pcie;
>> + struct dw_pcie *pci;
>> +
>> + if (pcie_cfg && pcie_cfg->firmware_managed) {
>> + /*
>> + * Firmware owns the link teardown and clock/PHY shutdown in
>> + * this mode; Linux only owns the chained MSI IRQ(s), which
>> + * still need to be masked off before shutdown proceeds.
>> + */
>> + pci = platform_get_drvdata(pdev);
>> + } else {
>> + pcie = platform_get_drvdata(pdev);
> This is no less than an intriguing choice.. please store the same
> element in both cases so we don't have to do this
for firmware_managed solutions struct qcom_pcie *pcie is not being
used/allocated at all. it will waste of memory if we allocate memory for just
this use case. - Krishna Chaitanya.
>> + pci = pcie->pci;
>> +
>> + if (pcie->global_irq)
>> + disable_irq(pcie->global_irq);
> This changed the ordering of disabling things versus the previous
> revision, although I don't think it should matter given this
> IRQ currently only services the linkdown event
>
>
> Konrad
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback
2026-09-09 12:40 ` Krishna Chaitanya Chundru
@ 2026-09-09 13:18 ` Konrad Dybcio
2026-09-10 3:26 ` Krishna Chaitanya Chundru
0 siblings, 1 reply; 13+ messages in thread
From: Konrad Dybcio @ 2026-09-09 13:18 UTC (permalink / raw)
To: Krishna Chaitanya Chundru, Jingoo Han, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas
Cc: linux-pci, linux-kernel, linux-arm-msm
On 9/9/26 2:40 PM, Krishna Chaitanya Chundru wrote:
>
>
> On 9/9/2026 6:06 PM, Konrad Dybcio wrote:
>> On 9/9/26 2:24 PM, Krishna Chaitanya Chundru wrote:
>>> From: Manivannan Sadhasivam <mani@kernel.org>
>>>
>>> PCIe host controllers should bring the link down cleanly before system
>>> shutdown/reboot proceeds to remove power/clocks from the controller.
>>> Without this, the link may still be up and endpoints still have
>>> transactions in flight when power/clocks are cut, which can trip SMMU
>>> translation faults or NoC protocol errors.
>>>
>>> Reuse dw_pcie_suspend_noirq() in the shutdown path to force the link
>>> into L2, putting it into D3cold.
>>>
>>> device_shutdown() runs with interrupts enabled, unlike suspend_noirq().
>>> Mask the chained MSI IRQ(s) and the Global IRQ before tearing down the
>>> link and clocks/PHY, since a late/spurious interrupt could otherwise
>>> reach a handler that touches now-unclocked PARF/DBI registers.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>> ---
>> [...]
>>
>>> +static void qcom_pcie_shutdown(struct platform_device *pdev)
>>> +{
>>> + const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
>>> + struct qcom_pcie *pcie;
>>> + struct dw_pcie *pci;
>>> +
>>> + if (pcie_cfg && pcie_cfg->firmware_managed) {
>>> + /*
>>> + * Firmware owns the link teardown and clock/PHY shutdown in
>>> + * this mode; Linux only owns the chained MSI IRQ(s), which
>>> + * still need to be masked off before shutdown proceeds.
>>> + */
>>> + pci = platform_get_drvdata(pdev);
>>> + } else {
>>> + pcie = platform_get_drvdata(pdev);
>> This is no less than an intriguing choice.. please store the same
>> element in both cases so we don't have to do this
> for firmware_managed solutions struct qcom_pcie *pcie is not being
> used/allocated at all. it will waste of memory if we allocate memory for just
> this use case. - Krishna Chaitanya.
Should we move it to a separate file/driver then, perhaps?
We could share the ECAM ops in e.g. pcie-qcom-common.c
Konrad
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback
2026-09-09 13:18 ` Konrad Dybcio
@ 2026-09-10 3:26 ` Krishna Chaitanya Chundru
0 siblings, 0 replies; 13+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-09-10 3:26 UTC (permalink / raw)
To: Konrad Dybcio, Manivannan Sadhasivam
Cc: linux-pci, linux-kernel, linux-arm-msm, Lorenzo Pieralisi,
Jingoo Han, Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
On 9/9/2026 6:48 PM, Konrad Dybcio wrote:
> On 9/9/26 2:40 PM, Krishna Chaitanya Chundru wrote:
>>
>> On 9/9/2026 6:06 PM, Konrad Dybcio wrote:
>>> On 9/9/26 2:24 PM, Krishna Chaitanya Chundru wrote:
>>>> From: Manivannan Sadhasivam <mani@kernel.org>
>>>>
>>>> PCIe host controllers should bring the link down cleanly before system
>>>> shutdown/reboot proceeds to remove power/clocks from the controller.
>>>> Without this, the link may still be up and endpoints still have
>>>> transactions in flight when power/clocks are cut, which can trip SMMU
>>>> translation faults or NoC protocol errors.
>>>>
>>>> Reuse dw_pcie_suspend_noirq() in the shutdown path to force the link
>>>> into L2, putting it into D3cold.
>>>>
>>>> device_shutdown() runs with interrupts enabled, unlike suspend_noirq().
>>>> Mask the chained MSI IRQ(s) and the Global IRQ before tearing down the
>>>> link and clocks/PHY, since a late/spurious interrupt could otherwise
>>>> reach a handler that touches now-unclocked PARF/DBI registers.
>>>>
>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>> [...]
>>>
>>>> +static void qcom_pcie_shutdown(struct platform_device *pdev)
>>>> +{
>>>> + const struct qcom_pcie_cfg *pcie_cfg = of_device_get_match_data(&pdev->dev);
>>>> + struct qcom_pcie *pcie;
>>>> + struct dw_pcie *pci;
>>>> +
>>>> + if (pcie_cfg && pcie_cfg->firmware_managed) {
>>>> + /*
>>>> + * Firmware owns the link teardown and clock/PHY shutdown in
>>>> + * this mode; Linux only owns the chained MSI IRQ(s), which
>>>> + * still need to be masked off before shutdown proceeds.
>>>> + */
>>>> + pci = platform_get_drvdata(pdev);
>>>> + } else {
>>>> + pcie = platform_get_drvdata(pdev);
>>> This is no less than an intriguing choice.. please store the same
>>> element in both cases so we don't have to do this
>> for firmware_managed solutions struct qcom_pcie *pcie is not being
>> used/allocated at all. it will waste of memory if we allocate memory for just
>> this use case. - Krishna Chaitanya.
> Should we move it to a separate file/driver then, perhaps?
> We could share the ECAM ops in e.g. pcie-qcom-common.c
I will let Mani to comment on this.
- Krishna Chaitanya.
>
> Konrad
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot
2026-09-09 12:24 [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Krishna Chaitanya Chundru
` (2 preceding siblings ...)
2026-09-09 12:24 ` [PATCH v6 3/3] PCI: qcom: Implement shutdown() callback Krishna Chaitanya Chundru
@ 2026-09-09 12:38 ` Konrad Dybcio
2026-09-09 12:42 ` Krishna Chaitanya Chundru
3 siblings, 1 reply; 13+ messages in thread
From: Konrad Dybcio @ 2026-09-09 12:38 UTC (permalink / raw)
To: Krishna Chaitanya Chundru, Jingoo Han, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas
Cc: linux-pci, linux-kernel, linux-arm-msm
On 9/9/26 2:24 PM, Krishna Chaitanya Chundru wrote:
> During system shutdown/reboot, power/clocks to the PCIe controller get
> removed regardless of link state. If the link is still up when that
> happens, it can trigger SMMU or NoC errors.
>
> This series adds a shutdown() callback to the Qualcomm PCIe host driver
> that forces the link into L2/D3cold before shutdown proceeds, reusing
> the existing suspend_noirq() path.
>
> Patch 1 fixes pci_host_common_d3cold_possible()'s underlying bus walk,
> which aborts as soon as it finds a device outside D3hot and can
> therefore miss a later PME-capable device -- something that becomes
> common once patch 3 starts forcing D3cold entry during shutdown while
> endpoints may still be in D0.
>
> Patch 2 adds a force parameter to dw_pcie_suspend_noirq() so that callers can
> set to make dw_pcie_suspend_noirq() force L2 entry during shutdown/
> reboot, skipping the D3cold capability check that can otherwise leave
> the link up if any endpoint hasn't suspended yet.
>
> Patch 3 adds qcom_pcie_shutdown() and wires it up as .shutdown.
>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
> Changes in v6:
> - remove return 0 in d3cold_possible() (konrad)
I re-read it once again and I'm not sure my comment was right..
Konrad
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot
2026-09-09 12:38 ` [PATCH v6 0/3] PCI: qcom: Implement shutdown() to avoid SMMU/NoC errors on reboot Konrad Dybcio
@ 2026-09-09 12:42 ` Krishna Chaitanya Chundru
0 siblings, 0 replies; 13+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-09-09 12:42 UTC (permalink / raw)
To: Konrad Dybcio, Jingoo Han, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas
Cc: linux-pci, linux-kernel, linux-arm-msm
On 9/9/2026 6:08 PM, Konrad Dybcio wrote:
> On 9/9/26 2:24 PM, Krishna Chaitanya Chundru wrote:
>> During system shutdown/reboot, power/clocks to the PCIe controller get
>> removed regardless of link state. If the link is still up when that
>> happens, it can trigger SMMU or NoC errors.
>>
>> This series adds a shutdown() callback to the Qualcomm PCIe host driver
>> that forces the link into L2/D3cold before shutdown proceeds, reusing
>> the existing suspend_noirq() path.
>>
>> Patch 1 fixes pci_host_common_d3cold_possible()'s underlying bus walk,
>> which aborts as soon as it finds a device outside D3hot and can
>> therefore miss a later PME-capable device -- something that becomes
>> common once patch 3 starts forcing D3cold entry during shutdown while
>> endpoints may still be in D0.
>>
>> Patch 2 adds a force parameter to dw_pcie_suspend_noirq() so that callers can
>> set to make dw_pcie_suspend_noirq() force L2 entry during shutdown/
>> reboot, skipping the D3cold capability check that can otherwise leave
>> the link up if any endpoint hasn't suspended yet.
>>
>> Patch 3 adds qcom_pcie_shutdown() and wires it up as .shutdown.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>> Changes in v6:
>> - remove return 0 in d3cold_possible() (konrad)
> I re-read it once again and I'm not sure my comment was right..
If we return directly without checking the pme capability we might disable
power to the endpoint and wake up may be broken.
I taught you pointed that mistake only. we need to check pme capability even
when device is not in d3hot.
- Krishna Chaitanya.
>
> Konrad
^ permalink raw reply [flat|nested] 13+ messages in thread