Re: Warning seen when removing a module using irqdomain framework

[pheragu]

On 2019-07-23 23:51, Marc Zyngier wrote:
> On Tue, 23 Jul 2019 14:52:34 -0700
> pheragu@xxxxxxxxxxxxxx wrote:
>
> Hi Prakruthi,
>
> > Hi,
> >
> > I have been working on a interrupt controller driver that uses tree
> > based mapping for its domain (irq_domain_add_tree(..)).
> > If I understand correctly, the clients get a mapping when they call
> > platform_get_irq(..). However, after these clients are removed
> > (rmmod), when I try to remove the interrupt controller driver where
> > it calls irq_domain_remove(..), I hit this warning from
> > kernel/kernel/irq/irqdomain.c:: irq_domain_remove(..)
> > [WARN_ON(!radix_tree_empty(&domain->revmap_tree));]-
> > WARNING: CPU: 0 PID: 238 at /kernel/kernel/irq/irqdomain.c:246 
> > irq_domain_remove+0x84/0x98
> >
> > Also, I see that the requested IRQs by the clients are still present
> > (in /proc/interrupts) even after they had been removed. Hence, I just
> > wanted to know how to handle this warning. Should the client clean up
> > by calling irq_dispose_mapping(..) or is it the responsibility of the
> > interrupt controller driver to dispose the mappings one by one?
>
> In general, building interrupt controller drivers as a module is a
> pretty difficult thing to do in a safe manner. As you found out, this
> relies on the irq_domain being "emptied" before it can be freed. There
> are some other gotchas in the rest of the IRQ stack as well.
>
> Doing that is hard. One of the reasons is that the OF subsystem will
> happily allocate all the interrupts it can even if there is no driver
> having requested them (see of_platform_populate). This means that you
> cannot track whether a client driver is using one of the interrupt your
> irqchip is in charge of. You can apply some heuristics, but they are in
> general all wrong.
>
> Fixing the OF subsystem is possible, but will break a lot of platforms
> that will have to be identified and fixed one by one.  Another
> possibility would be to refcount irqdescs, and make sure the irqdomain
> directly holds pointers to them. Doable, but may create overhead.
>
> To sum it up, don't build your irqchip driver as a module if you can
> avoid it. If you can't, you'll have to be very careful about how the
> mapping is established (make sure it is not created by
> of_platform_populate), and use irq_dispose_mapping in the client
> drivers.
As per your suggestion I tried making this driver a statically compiled 
one.
I tried various approaches with this -

1. Using arch_inticall(..) - When I used this call, I saw that once the
clients were removed, I don't see the IRQs requested by them (in 
/proc/interrupts).

2. Using module_init(..) (statically compiled) - When I used this call, 
I saw that
even after the clients were removed, I do see their requested IRQs in 
/proc/interrupts.
This behavior in #2 is the same as the one I saw when I compiled my 
driver as a module
and used arch_initcall(..).

Is there any reason why this is seen only with arch_initcall(..) used 
statically?


Regards,
Prakruthi Deepak