On Sat, Jul 13 2024 at 14:32, Bjorn Helgaas wrote:
> On Sat, Jul 13, 2024 at 12:33:29PM +0200, Thomas Gleixner wrote:
>> > 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?
>>
>> Right.
>>
>> But the real question is why is such a mapping not torn down by the
>> entity (device, bridge, whatever) which set it up in the first place?
>
> Marek/Pali, the commit log mentions a crash when unloading. Do you
> have a pointer to any details? Maybe there's a driver there that we
> can fix?
Pali already explained it in his reply to me, but for some stupid reason
(my fault hitting reply instead of reply-all) this did not make the list.
Let me replay the discussion:
>>> Pali:
>>> I'm not expert neither. But IIRC it is because endpoint drivers do not
>>> call irq_dispose_mapping at their own for shared IRQs. And this is how
>>> shared PCI INTA-D interrupts are implemented in the kernel. First
>>> endpoint driver (e.g. wifi card) request for shared interrupt and kernel
>>> automatically creates irq mapping if it does not exist. Second endpoint
>>> driver (e.g. second wifi card) request for shared interrupt and kernel
>>> just returns existing mapping. And when the first endpoint driver is
>>> releasing its own irq handler it do not dispose irq mapping because it
>>> may be shared with other endpoint drivers (in this case with second wifi
>>> card). Seems that the owner of these shared mappings is the PCI
>>> controller driver and it has to do dispose them on its own removal (when
>>> releasing the domain for shared PCI IRQs).
>> tglx:
>> Working around this in a particular PCI controller driver is just wrong
>> then. This wants a common cleanup function so all affected drivers which
>> remove the INTX irqdomain can be fixed up without copy & pasta of the
>> very same code. Something like the below and then change the
>> irq_domain_remove() call for the INTX domain to use that function.
There was some additional back and forth but the actual patch which can
be used for both INTX and other, e.g. error reporting, domains is below.
Sorry for taking this offlist unintenitonally.
The background of all this is that initially PCI[e] controllers were not
removable/modular. Later on the whole modularization effort created this
problem.
Thanks,
tglx
---
--- a/drivers/pci/irq.c
+++ b/drivers/pci/irq.c
@@ -278,3 +278,19 @@ int __weak pcibios_alloc_irq(struct pci_
void __weak pcibios_free_irq(struct pci_dev *dev)
{
}
+
+#ifdef CONFIG_IRQDOMAIN
+void pci_remove_irqdomain(struct irqdomain *domain, unsigned int nr_irqs)
+{
+ /*
+ * Comment explaining the oddity of this.
+ */
+ for (unsigned int i = 0; j < nr_irqs; i++) {
+ int virq = irq_find_mapping(domain, i);
+
+ if (virq > 0)
+ irq_dispose_mapping(virq);
+ }
+ irq_domain_remove(domain);
+}
+#endif
