[+cc Pali, seems like the author should be included,
Thomas, Marc since they actually know about IRQs, unlike me]
On Thu, Jul 11, 2024 at 03:25:44PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@xxxxxxxxxx>
>
> The documentation for the irq_domain_remove() function says that all
> mappings within the IRQ domain must be disposed before the domain is
> removed.
>
> Currently, the INTx IRQs are not disposed in pci-mvebu driver .remove()
> method, which causes the kernel to crash when unloading the driver and
> then reading /sys/kernel/debug/irq/irqs/<num> or /proc/interrupts.
>
> Unmapping of the IRQs at this point of the .remove() method is safe,
> since the PCIe bus is already unregistered, and all its devices are
> unbound from their drivers and removed. If there was indeed any
> remaining use of PCIe resources, then it would mean that PCIe hotplug
> code is broken, and we have bigger problems.
>
> Fixes: ec075262648f ("PCI: mvebu: Implement support for legacy INTx interrupts")
> Reported-by: Hajo Noerenberg <hajo-linux-bugzilla@xxxxxxxxxxxxx>
Is there a URL for this report?
> Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> Reviewed-by: Marek Behún <kabel@xxxxxxxxxx>
> [ Marek: refactored a little, added more explanation to commit message ]
> Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> ---
> Changes since v1:
> - added explanation into commit message about why this is safe to do,
> as suggested by Andy. The explanation originally comes from Pali:
> https://lore.kernel.org/linux-arm-kernel/20220809133911.hqi7eyskcq2sojia@pali/
> ---
> drivers/pci/controller/pci-mvebu.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 29fe09c99e7d..91a02b23aeb1 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -1683,8 +1683,15 @@ static void mvebu_pcie_remove(struct platform_device *pdev)
> irq_set_chained_handler_and_data(irq, NULL, NULL);
>
> /* Remove IRQ domains. */
> - if (port->intx_irq_domain)
> + if (port->intx_irq_domain) {
> + for (int j = 0; j < PCI_NUM_INTX; j++) {
> + int virq = irq_find_mapping(port->intx_irq_domain, j);
> +
> + if (virq > 0)
> + irq_dispose_mapping(virq);
I am not an IRQ expert, so all I can really do is compare this to
usage in other drivers.
There are 20+ drivers in drivers/pci/controller, and I don't see
irq_dispose_mapping() usage similar to this elsewhere. Does that mean
most or all of the other drivers have a similar defect?
> + }
> irq_domain_remove(port->intx_irq_domain);
> + }
>
> /* Free config space for emulated root bridge. */
> pci_bridge_emul_cleanup(&port->bridge);
> --
> 2.44.2
>
