On Tue, Sep 24, 2013 at 02:20:44PM +0200, Linus Walleij wrote:

On Mon, Sep 23, 2013 at 10:29 PM, Thierry Reding
[off-list ref] wrote:

quoted

On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote:

quoted

quoted

I think it is better to first go over the call sites and make them
all handle negative return numbers rather than pushing the
obscure __interface.

(...)

quoted

Well, the problem is that the current patch changes the signature of the
function as well, therefore the call sites will have to be updated all
at once in a single patch to avoid build breakage.

Hm yeah OK I see the problem, but can we atleast avoid the
__thing? Like calling the new function irq_create_mapping_strict()
or whatever.

_strict sort of implies that it does something more than the non-strict
irq_create_mapping() while it really doesn't. Perhaps the alternative
proposed below would indeed be a better solution.

quoted

Another alternative could be to change the signature in a way that does
not break compatibility. For instance I think it could work out if we
change this function to return int instead of unsigned int but keep the
same semantics to begin with (return 0 on failure). Then update all call
sites to handle potential negative errors and after that return negative
error codes.

Hm that sounds like an attractive solution to me actually.

The only thing we'd loose is the additional bit, but given that most (if
not all) platforms that use DT are 32-bit (do we actually support any
platforms that don't have 32-bit integers?) that should not matter at
all. We're not very likely to get anywhere near that number of
interrupts in the system.

quoted

That still wouldn't catch any callers introduced between
the patch creation and application.

Such things happen all the time, just have to be attentive in
what goes into linux-next...

Given that linux-next might not be with us for much longer before the
3.12 release, I'm thinking of deferring the series until then. Or at
least trying to get it merged. Otherwise we'll probably have to deal
with a lot of fall out during the merge window.

In any case, it'd be nice to get some feedback on the general idea of
the patch series from other people involved. I'd hate to do all the
conversions just to have it NAKed at the last minute.

Another minor thing:

+static int __irq_create_mapping(struct irq_domain *domain,
+                               irq_hw_number_t hwirq, unsigned int *virqp)

Unless you can make a very good case for why there should
be a "v" in the beginning of virqp, then remove it and call it
"irqp" simply.

All Linux IRQs are virtual and we're already clearly separating
out those that are not by calling them "hwirq".

Yeah, I was just trying to adapt to what was already there. But with the
alternative proposal that'll go away anyway.

Thierry