Hi John,
On 2020-11-23 12:54, John Garry wrote:
Hi Marc,
Right, but if the driver is removed then the interrupts should be
deallocated, right?
When removing the driver we just call free_irq(), which removes the
handler and disables the interrupt.
But about the irq_desc, this is created when the mapping is created
in
irq_create_fwspec_mapping(), and I don't see this being torn down in
the driver removal, so persistent in that regard.
If the irq_descs are created via the platform_get_irq() calls in
platform_get_irqs_affinity(), I'd expect some equivalent helper to
tear things down as a result, calling irq_dispose_mapping() behind the
scenes.
So this looks lacking in the kernel AFAICS...
So is there a reason for which irq dispose mapping is not a
requirement for drivers when finished with the irq? because of shared
interrupts?
For a bunch of reasons: IRQ number used to be created 1:1 with their
physical counterpart, so there wasn't a need to "get rid" of the
associated data structures. Also, people expected their drivers
to be there for the lifetime of the system (believe it or not,
hotplug devices are pretty new!).
Shared interrupts are just another part of the problem (although we
should be able to work out whether there is any action attached to
the descriptor before blowing it away.
So for pci msi I can see that we free the irq_desc in
pci_disable_msi() -> free_msi_irqs() -> msi_domain_free_irqs() ...
So what I am missing here?
I'm not sure the paths are strictly equivalent. On the PCI side, we
can have something that completely driver agnostic, as it is all
architectural. In your case, only the endpoint driver knows about what
happens, and needs to free things accordingly.
Finally, there is the issue in your driver that everything is
requested using devm_request_irq, which cannot play nicely with an
explicit irq_desc teardown. You'll probably need to provide the
equivalent devm helpers for your driver to safely be taken down.
Yeah, so since we use the devm irq request method, we also need a devm
dispose release method as we can't dispose the irq mapping in the
remove() method, prior to the irq_free() in the later devm release
method.
But it looks like there is more to it than that, which I'm worried is
far from non-trivial. For example, just calling irq_dispose_mapping()
for removal and then plaform_get_irq()->acpi_get_irq() second time
fails as it looks like more tidy-up is needed for removal...
Most probably. I could imagine things failing if there is any trace
of an existing translation in the ITS or in the platform-MSI layer,
for example, or if the interrupt is still active...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
