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

FTR,

On Tue, May 5, 2026 at 7:24 PM <[email protected]> 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 <[email protected]>
>
> 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