Re: [BUG] irq_dispose_mapping after irq request failure

[Michael Ellerman <michael@ellerman.id.au>]

On Tue, Feb 12, 2013 at 11:51:13AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2013-02-11 at 20:52 +0000, Grant Likely wrote:
> > Really the irq mappings should be using reference counting. The existing
> > code is naive on this count and just releases the irq on the first call
> > to irq_dispose_mapping().  I've not gotten around to fixing that. Anyone
> > want to take that task on?
> 
> Is this the best approach ?
> 
> The original idea was that there was no point disposing of mappings in most
> cases and keeping the mapping around would provide a bit of stability of
> interrupt numbers which might come in handy for debugging etc...
> 
> The few cases where disposing of a mapping might be useful is if the underlying
> physical interrupts completely disappear, as in a cascaded controller gets
> removed or that sort of thing, which is a very rare case... And even then...

That may have been the intent, but we forgot to tell driver writers, ourselves
included.

> Could you just make irq_dispose_mapping() check if the irq desc is
> active and fail/WARN/BUG if it is ? I don't see the point of adding a refcount,
> that feels overkill.

I don't think you can, "active" is not well defined. Other code may have
done nothing other than create the mapping and remembered the virq,
which will break if you destroy the mapping. Or?

I agree refcounting is not fun. It'll end up with the same mess as
of_node_get/put() where practically every 2nd piece of code leaks
references.

I guess we can't go the other way, and say that mapping the same hwirq
twice is an error.

cheers