From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v4 1/6] xen: dt: add dt_for_each_irq_map
 helper
Date: Fri, 3 Jul 2015 15:16:54 +0100
Message-ID: <1435933014.9447.123.camel@citrix.com>
References: <1431084401.2660.440.camel@citrix.com>
	<1431084420-14372-1-git-send-email-ian.campbell@citrix.com>
	<558D9040.7070503@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <558D9040.7070503@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@citrix.com>
Cc: stefano.stabellini@eu.citrix.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.