| Message ID | 20240619142829.2804-1-kabel@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: mvebu: Dispose INTx irqs prior to removing INTx domain | expand |
+ Marc On Wed, Jun 19, 2024 at 04:28:29PM +0200, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > Documentation for irq_domain_remove() says that all mapping within the > domain must be disposed prior to domain remove. > > Currently INTx irqs are not disposed in pci-mvebu.c device unbind callback > which cause that kernel crashes after unloading driver and trying to read > /sys/kernel/debug/irq/irqs/<num> or /proc/interrupts. > Thanks a lot for respinning this patch. It is indeed fixing a real problem since this driver can be unloaded runtime. It is always a debate on whether an irqchip controller should be allowed to be removed runtime or not, but I hope Marc will provide some inputs here. > Fixes: ec075262648f ("PCI: mvebu: Implement support for legacy INTx interrupts") > Reported-by: Hajo Noerenberg <hajo-linux-bugzilla@noerenberg.de> > Signed-off-by: Pali Rohár <pali@kernel.org> > Reviewed-by: Marek Behún <kabel@kernel.org> > [ refactored a little ] > Signed-off-by: Marek Behún <kabel@kernel.org> But this patch looks good to me. I hope that we should be able to use this patch as a precedent for other drivers as well. Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > This was discussed back in 2022 > https://lore.kernel.org/linux-arm-kernel/20220709161858.15031-1-pali@kernel.org/ > IMO Pali gave good arguments about why it should be applied, and Lorenzo > agreed. > > Can we get this applied? > --- > 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); > + } > irq_domain_remove(port->intx_irq_domain); > + } > > /* Free config space for emulated root bridge. */ > pci_bridge_emul_cleanup(&port->bridge); > -- > 2.44.2 > >
On Wed, Jun 19, 2024 at 04:28:29PM +0200, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > Documentation for irq_domain_remove() says that all mapping within the > domain must be disposed prior to domain remove. > > Currently INTx irqs are not disposed in pci-mvebu.c device unbind callback > which cause that kernel crashes after unloading driver and trying to read > /sys/kernel/debug/irq/irqs/<num> or /proc/interrupts. I was wondering why this is safe. Are there still users? If the users are being removed first, why are there still mappings. This is discussed in the old thread: > I think that in this case for pci-mvebu.c it is safe because: At the > first step of unbind procedure is called unregistraction of PCIe bus > with all devices bound on it. This ensures that all PCIe endpoint > drivers are unbind, devices removed and no new driver or device and > appear. After that there should not be any remaining usage of PCIe > resources (if there is then whole PCIe hotplug code is broken and we > have other and bigger issue...). Next pci-mvebu.c manually disposes > all remaining legacy interrupts (which PCI core code does not > because legacy interrupts are shared and it does not know if they > are used or not). This is safe because at these stage there are no > PCI drivers bound, there is no PCI device for that controller > registered. And after that is removed IRQ domain (which has finally > disposed all interrupts). If there is a need to respin, it would be good to include this in the commit message. Andrew
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); + } irq_domain_remove(port->intx_irq_domain); + } /* Free config space for emulated root bridge. */ pci_bridge_emul_cleanup(&port->bridge);