Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Fri, Jan 06, 2012 at 05:20:16PM +0100, Thierry Reding wrote:
> * Grant Likely wrote:
> > Hi Thierry,
> > 
> > On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding
> > <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > > The irq_domain_add() function needs the number of interrupts in the
> > > domain to properly initialize them. In addition the allocated domain
> > > is now returned by the irq_domain_{add,generate}_simple() helpers.
> > 
> > The commit text should also include the justification for renaming
> > irq_domain_create_simple() -> irq_domain_add_simple()
> 
> Actually the commit only fixes up the comment. The function has always been
> called irq_domain_add_simple().
> 
> For reference, this was introduced in commit 7e71330.

Hahaha.  Oops, you're right.  :-)

> 
> > >        domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> > > -       if (!domain) {
> > > -               WARN_ON(1);
> > > -               return;
> > > -       }
> > > +       if (!domain)
> > > +               return ERR_PTR(-ENOMEM);
> > 
> > Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO).
> 
> Returning NULL here is probably okay. Can the ERR_PTR stay in
> irq_domain_generate_simple(), though? It has two error conditions and
> handling both by returning NULL may not be what we want.

No.  ERR_PTR is a horrible pattern because you cannot tell by looking
at a prototype that returns a pointer whether or not the correct
failure test is "if (!ptr)" or "if (IS_ERR(ptr))".  Unless it is
absolutely critical for an error code to be returned (which isn't the
case here) I will not accept new code that uses ERR_PTR().

In this case, if irq_domain_add_simple() fails, then something is very
wrong.  I'd much rather the routine complain loudly regardless of the
error condition.

Actually, looking again at irq_domain_generate_simple() it should
probably succeed even if it cannot find a matching node since an
irq_domain does more than just device tree translation.  Although,
irq_domain_generate_simple() is a stop-gap solution that will
eventually be removed.

g.