Re: [PATCH] PCI: mediatek-gen3: Initialize all resources before installing ISR

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete Fix / NULL Pointer Dereference in MSI handler.

Pre-existing issues:
- [High] Lifecycle / SError Crash in teardown paths.
- [High] Concurrency / Hard Lockup due to unmapped bootloader interrupts.
--

--- Patch [1]: [PATCH] PCI: mediatek-gen3: Initialize all resources before installing ISR ---
commit d73aca9231cdee14919c7ffe3c94da11dd9f840a
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: PCI: mediatek-gen3: Initialize all resources before installing ISR

This commit modifies the probe sequence to initialize resources before
installing the IRQ handler. It aims to prevent a NULL pointer dereference
when a pending bootloader interrupt fires before the controller structure
is fully initialized.

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index bcbd08c82a5ce..f8a98c0b2c453 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
[ ... ]
> @@ -1208,6 +1189,9 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>  		}
>  	}
>  
> +	/* All resources are now available: install the chained handler */
> +	irq_set_chained_handler_and_data(pcie->irq, mtk_pcie_irq_handler, pcie);
> +

[Severity: High]
Does this code initialize msi_set->base before the handler is installed?

The commit message states that all required resources are initialized before
installing the interrupt handler. However, if the bootloader leaves a pending
MSI interrupt, mtk_pcie_irq_handler() will immediately call
mtk_pcie_msi_handler().

It appears msi_set->base is not initialized until mtk_pcie_enable_msi()
is called inside mtk_pcie_startup_port():

mtk_pcie_startup_port() {
    ...
    mtk_pcie_enable_msi() {
        msi_set->base = pcie->base + ...
    }
}

Would this sequence cause mtk_pcie_msi_handler() to dereference an
uninitialized pointer at msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET?

[Severity: High]
This is a pre-existing issue, but does this installation safely handle
unmapped bootloader interrupts?

If the bootloader leaves an interrupt pending before child devices have
probed and mapped their IRQs, generic_handle_domain_irq() will fail and
bypass the irq_ack callback.

For MSI interrupts, the hardware status bit would never be cleared. Could
this cause the following loop in mtk_pcie_msi_handler() to spin infinitely
and hard-lock the CPU?

	do {
		msi_status = readl_relaxed(msi_set->base +
					   PCIE_MSI_SET_STATUS_OFFSET);
		msi_status &= msi_enable;
		if (!msi_status)
			break;
        ...
	} while (true);

Should we explicitly clear pending hardware interrupts before registering
the chained handler?

>  	/* Try link up */
>  	err = mtk_pcie_startup_port(pcie);
>  	if (err)
[ ... ]
> @@ -1248,13 +1235,14 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  		goto err_tear_down_irq;
>  	}
>  
> +	host->ops = &mtk_pcie_ops;
> +	host->sysdata = pcie;
> +	platform_set_drvdata(pdev, pcie);
> +
>  	err = mtk_pcie_setup(pcie);
>  	if (err)
>  		goto err_destroy_pwrctrl;
>  
> -	host->ops = &mtk_pcie_ops;
> -	host->sysdata = pcie;
> -
>  	err = pci_host_probe(host);
>  	if (err)
>  		goto err_power_down_pcie;

[Severity: High]
This is a pre-existing issue, but could the error teardown sequence cause
an SError crash?

If pci_host_probe() fails, the code jumps to err_power_down_pcie.
Looking at the error path execution:

err_power_down_pcie:
	mtk_pcie_devices_power_down(pcie);
	mtk_pcie_power_down(pcie);
err_destroy_pwrctrl:
    ...
err_tear_down_irq:
	mtk_pcie_irq_teardown(pcie);

The hardware is powered down before mtk_pcie_irq_teardown() removes the
chained IRQ handler.

As noted in the commit message, modifying or reading registers when the
hardware is powered down can result in an SError. If a shared or spurious
interrupt fires during this window, wouldn't mtk_pcie_irq_handler() read
from PCIE_INT_STATUS_REG while the hardware is powered off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716113142.46742-1-angelogioacchino.delregno@collabora.com?part=1