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

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 08/05/2015 13:26, Ian Campbell wrote:
> This function iterates over a nodes interrupt-map property and calls a
> callback for each interrupt. For now it only supplies the translated
> IRQ since my use case has no need of e.g. child unit address. These
> can be added as needed by any future users.
>
> This follows much the same logic as dt_irq_map_raw when parsing the
> interrupt-map, but doesn't walk up the tree doing the actual
> translation and it iterates over all entries instead of just looking
> for the first match.
>
> I looked into refactoring dt_irq_map_raw but I couldn't find a way
> which I was confident in, plus I was reluctant to diverge from the
> Linux roots of this function any further.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Julien Grall <julien.grall@citrix.com>
> ---
> v4: Pass a dt_irq not a dt_irq_raw to the callback.
> ---
>   xen/common/device_tree.c      |  148 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h |   12 ++++
>   2 files changed, 160 insertions(+)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 02cae91..a2aeecd 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -811,6 +811,154 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
>       return (psize / onesize);
>   }
>
> +int dt_for_each_irq_map(const struct dt_device_node *dev,
> +                        int (*cb)(const struct dt_device_node *,
> +                                  const struct dt_irq *,
> +                                  void *),
> +                        void *data)
> +{
> +    const struct dt_device_node *ipar, *tnode, *old = NULL;
> +    const __be32 *tmp, *imap;
> +    u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
> +    u32 imaplen;
> +    int i, ret;
> +
> +    struct dt_raw_irq dt_raw_irq;
> +    struct dt_irq dt_irq;
> +
> +    dt_dprintk("%s: par=%s cb=%p data=%p\n", __func__,
> +               dev->full_name, cb, data);
> +
> +    ipar = dev;
> +
> +    /* 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.

[..]

> +    if ( intsize > DT_MAX_IRQ_SPEC )
> +    {
> +        dt_dprintk(" -> too many irq specifier cells\n");
> +        goto fail;
> +    }

[..]

> +    /* Parse interrupt-map */
> +    while ( imaplen > (addrsize + intsize + 1) )
> +    {
> +        /* skip child unit address and child interrupt specifier */
> +        imap += addrsize + intsize;
> +        imaplen -= addrsize + intsize;
> +
> +        /* Get the interrupt parent */
> +        ipar = dt_find_node_by_phandle(be32_to_cpup(imap));

[..]

> +        /* Get #interrupt-cells and #address-cells of new
> +         * parent
> +         */
> +        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
> +        if ( tmp == NULL )
> +        {
> +            dt_dprintk(" -> parent lacks #interrupt-cells!\n");
> +            goto fail;
> +        }
> +        pintsize = be32_to_cpu(*tmp);

[..]

> +        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.

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.

Regards,

-- 
Julien Grall