[PATCH] PCI: mediatek-gen3: Do full device power down on removal

[PATCH] PCI: mediatek-gen3: Do full device power down on removal

[Chen-Yu Tsai]

When power control for downstream devices was introduced in the
mediatek-gen3 PCIe controller driver, only the power to the downstream
devices was cut when the controller driver is removed. This matched
existing behavior, but in hindsight a proper power down sequence should
have been followed.

Call mtk_pcie_devices_power_down() on driver removal so that in addition
to removing power from the downstream devices, PERST# is asserted.

Fixes: 1a152e21940a ("PCI: mediatek-gen3: Integrate new pwrctrl API")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index b0accd828589..e6acca7e79a0 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -1260,7 +1260,7 @@ static void mtk_pcie_remove(struct platform_device *pdev)
 	pci_remove_root_bus(host->bus);
 	pci_unlock_rescan_remove();
 
-	pci_pwrctrl_power_off_devices(pcie->dev);
+	mtk_pcie_devices_power_down(pcie);
 	mtk_pcie_power_down(pcie);
 	pci_pwrctrl_destroy_devices(pcie->dev);
 	mtk_pcie_irq_teardown(pcie);
-- 
2.54.0.545.g6539524ca2-goog

Re: [PATCH] PCI: mediatek-gen3: Do full device power down on removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Asserting PERST# and disabling clocks before deregistering the interrupt handler introduces a race condition that can cause a bus fault (kernel panic).
--

commit 392cdae7adde07f3a5a5cd47d941a62a1d778a2b
Author: Chen-Yu Tsai <wenst@chromium.org>

PCI: mediatek-gen3: Do full device power down on removal

This commit updates the driver removal path to follow a proper power down
sequence for downstream devices. It calls mtk_pcie_devices_power_down()
on driver removal so that PERST# is asserted.

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index b0accd8285892..e6acca7e79a0f 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -1260,7 +1260,7 @@ static void mtk_pcie_remove(struct platform_device *pdev)
>  	pci_remove_root_bus(host->bus);
>  	pci_unlock_rescan_remove();
>  
> -	pci_pwrctrl_power_off_devices(pcie->dev);
> +	mtk_pcie_devices_power_down(pcie);
>  	mtk_pcie_power_down(pcie);

Could this sequence cause a regression by introducing a race condition
with the interrupt handler?

Calling mtk_pcie_devices_power_down() asserts the PERST# pin, which forces
the downstream PCIe link down. This can trigger a link-down or MAC error
interrupt at the controller level.

Immediately following this, mtk_pcie_power_down() disables the transaction
layer clocks via clk_bulk_disable_unprepare().

If an interrupt fires during this window, the top-level chained interrupt
handler will execute because mtk_pcie_irq_teardown() hasn't been called yet.

If the handler attempts to read controller registers while its clocks are
disabled:

mtk_pcie_irq_handler()
	status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG);

Could this cause a synchronous external abort and crash the kernel?

>  	pci_pwrctrl_destroy_devices(pcie->dev);
>  	mtk_pcie_irq_teardown(pcie);

Should the interrupt handler be deregistered or masked before the clocks
are disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505105918.1823170-1-wenst@chromium.org?part=1

Re: [PATCH] PCI: mediatek-gen3: Do full device power down on removal

[Chen-Yu Tsai]

FTR,

On Tue, May 5, 2026 at 7:24 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Asserting PERST# and disabling clocks before deregistering the interrupt handler introduces a race condition that can cause a bus fault (kernel panic).
> --
>
> commit 392cdae7adde07f3a5a5cd47d941a62a1d778a2b
> Author: Chen-Yu Tsai <wenst@chromium.org>
>
> PCI: mediatek-gen3: Do full device power down on removal
>
> This commit updates the driver removal path to follow a proper power down
> sequence for downstream devices. It calls mtk_pcie_devices_power_down()
> on driver removal so that PERST# is asserted.
>
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index b0accd8285892..e6acca7e79a0f 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -1260,7 +1260,7 @@ static void mtk_pcie_remove(struct platform_device *pdev)
> >       pci_remove_root_bus(host->bus);
> >       pci_unlock_rescan_remove();
> >
> > -     pci_pwrctrl_power_off_devices(pcie->dev);
> > +     mtk_pcie_devices_power_down(pcie);
> >       mtk_pcie_power_down(pcie);
>
> Could this sequence cause a regression by introducing a race condition
> with the interrupt handler?
>
> Calling mtk_pcie_devices_power_down() asserts the PERST# pin, which forces
> the downstream PCIe link down. This can trigger a link-down or MAC error
> interrupt at the controller level.
>
> Immediately following this, mtk_pcie_power_down() disables the transaction
> layer clocks via clk_bulk_disable_unprepare().
>
> If an interrupt fires during this window, the top-level chained interrupt
> handler will execute because mtk_pcie_irq_teardown() hasn't been called yet.
>
> If the handler attempts to read controller registers while its clocks are
> disabled:
>
> mtk_pcie_irq_handler()
>         status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG);
>
> Could this cause a synchronous external abort and crash the kernel?
>
> >       pci_pwrctrl_destroy_devices(pcie->dev);
> >       mtk_pcie_irq_teardown(pcie);
>
> Should the interrupt handler be deregistered or masked before the clocks
> are disabled?

The driver only handles chained IRQs, i.e. the INTX and MSI interrupts.
By the time this function is called, the downstream PCIe devices should
all have been stopped and removed, meaning all chained interrupts would
have been disabled already.

ChenYu