Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc()

[Marc Zyngier <maz@kernel.org>]

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...