Re: [PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Thierry Reding]

On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 10:31 AM, Thierry Reding
> <thierry.reding@xxxxxxxxx> wrote:
> 
> > This is a version of irq_create_mapping() that propagates the precise
> > error code instead of returning 0 for all errors. It will be used in
> > subsequent patches to allow further propagation of error codes.
> >
> > To avoid code duplication, implement irq_create_mapping() as a wrapper
> > around the new __irq_create_mapping().
> >
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> 
> Surprise! I don't like this.
> 
> 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.
> 
> I know from patch 0 that you think it's too much to change these
> 127 call sites but I don't think so, and I'm happy to merge one
> big patch changing all the 20 users in drivers/gpio.
> 
> Likewise with the 11 consumers in drivers/pinctrl.
> 
> It's just a a few archs+subsystems and it's just plain work.

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. And that's excluding
any potential fallout from new callsites added between the creation of
the patch and its application.

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. That still wouldn't catch any callers introduced between
the patch creation and application.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature