Re: [PATCH v4 1/6] xen: dt: add dt_for_each_irq_map helper

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-06-26 at 19:47 +0200, Julien Grall wrote:

> > +    /* First get the #interrupt-cells property of the current cursor
> > +     * that tells us how to interpret the passed-in intspec. If there
> > +     * is none, we are nice and just walk up the tree
> > +     */
> > +    do {
> > +        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
> > +        if ( tmp != NULL )
> > +        {
> > +            intsize = be32_to_cpu(*tmp);
> > +            break;
> > +        }
> > +        tnode = ipar;
> > +        ipar = dt_irq_find_parent(ipar);
> > +    } while ( ipar );
> 
> This loop doesn't seem useful. AFAIU the spec, the PCI node (i.e your 
> variable dev) will always have property #interrupt-cells. We will break 
> directly.

The reason for this is explained in the comment i.e. "we are nice" to
broken trees.

This code is from Linux, via dt_irq_map_raw, I don't fancy messing with
it too much.

> > +        dt_raw_irq.controller = ipar;
> > +        dt_raw_irq.size = pintsize;
> 
> Don't you need to check that pintsize is < DT_MAX_IRQ_SPEC?
> The previous "if ( ... > DT_MAX_IRQ_SPEC )" will likely be done on a 
> different parent.

Yes, I think that would be a good idea.

> 
> For instance with the following incomplete DT (based on the 
> apm-storm.dsi in Linux):
> 
> pcie0 {
>     #interrupt-cells = <1>;
>     ...
>     #interrupt-map = < 0x0 0x0 0x0 0x1 &gic ... >
> }
> 
> The first ipar will point to pcie0 because it has a property 
> "#interrupt-cells", while the second time ipar will point to the gic node.
> 
> > +        for ( i = 0; i < pintsize; i++ )
> > +            dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
> > +
> > +        ret = dt_irq_translate(&dt_raw_irq, &dt_irq);
> > +        if ( ret < 0 )
> 
> The other caller of dt_irq_translate returns an error when ret is not 0. 
> I would do the same here.

dt_device_get_irq just returns the value of dt_irq_translate directly.

Are you suggesting this code should treat positive results as an error
as well as negative ones? I don't agree, this function has the normal 0
on success -ve on error semantics AFAICT.

Or did you mean something else?
Ian.