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:

quoted

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