Re: [PATCH 3/9] PCI: altera: Protect root bus removal with rescan lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup of chained IRQ handler and IRQ domain on probe failure paths in `altera_pcie_probe()`.
- [Medium] The driver does not disable hardware interrupt generation during device removal.
- [Medium] The driver erroneously calls `irq_dispose_mapping()` on the parent platform IRQ in `altera_pcie_irq_teardown()`.
--

commit c918986379b1a0013bed56503edcc992be2805fa
Author: Hans Zhang <18255117159@163.com>

PCI: altera: Protect root bus removal with rescan lock

This commit wraps pci_stop_root_bus() and pci_remove_root_bus() with
the rescan locks to avoid potential races with sysfs-triggered rescans
during device removal.

> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> index 3dbb7adc421c4..7e1db267ae342 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -1045,8 +1045,10 @@ static void altera_pcie_remove(struct platform_device *pdev)
>  	struct altera_pcie *pcie = platform_get_drvdata(pdev);
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);

[Severity: Medium]
This isn't a bug introduced by this patch, but does the driver need to
disable hardware interrupt generation during device removal?

The driver enables hardware interrupts by writing to device registers during
probe, but tears down the driver here without masking or disabling those
hardware interrupts.

Even though the chained handler is unregistered below, if the device still
asserts the interrupt line, could the CPU receive an interrupt with no
registered handler and cause the spurious interrupt detector to permanently
disable the IRQ line?

>  
> +	pci_lock_rescan_remove();
>  	pci_stop_root_bus(bridge->bus);
>  	pci_remove_root_bus(bridge->bus);
> +	pci_unlock_rescan_remove();
>  	altera_pcie_irq_teardown(pcie);

[Severity: Medium]
This is a pre-existing issue, but does altera_pcie_irq_teardown()
incorrectly dispose of the platform IRQ mapping?

The teardown function executes irq_dispose_mapping(pcie->irq). However,
pcie->irq is retrieved via platform_get_irq(), meaning the mapping was
created and is owned by the platform subsystem.

Device drivers usually shouldn't destroy IRQ mappings they did not
explicitly allocate. Could disposing of this mapping break the lifecycle
management of the platform IRQ if the driver is rebound?

[Severity: High]
This is also a pre-existing issue not introduced here, but I noticed a
potential use-after-free in the probe error paths.

The probe function successfully registers a chained IRQ handler via
irq_set_chained_handler_and_data(pcie->irq, pcie->pcie_data->ops->rp_isr,
pcie). If a subsequent step like altera_pcie_init_irq_domain() or
pci_host_probe() fails, the function returns an error without unregistering
the chained handler.

Since the pcie structure is managed by devres, it is freed upon probe
failure. If the hardware asserts the interrupt line, could the core IRQ code
invoke the chained handler and dereference the freed pcie pointer?

>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521161822.132996-1-18255117159@163.com?part=3