Re: [Xen-devel] [RFC PATCH 2/2] xen/device-tree: Add ability to handle nodes with interrupts-extended prop

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 02/05/2019 15:13, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> Xen expects to see "interrupts" property when parsing host
> device-tree. But, there are cases when some device nodes contain
> "interrupts-extended" property instead.
>
> The good example here is arch timer node for R-Car Gen3/Gen2 family,
> which is mandatory device for Xen usage on ARM. And without ability
> to handle such nodes, Xen fails to operate:
Per the binding documentation [1], the interrupts-extend property should only be 
used when a device has multiple interrupt parents. This is not the case of the 
arch timer, so why is it used there?
Don't get me wrong, I am fine with the idea of adding "interrupts-extend". 
However, the commit message should give some ground why a new property has been 
introduced/used over the current one.
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Timer: Unable to retrieve IRQ 0 from the device tree
> (XEN) ****************************************
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
>   xen/common/device_tree.c | 32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 65862b5..00ada6e 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -987,9 +987,19 @@ unsigned int dt_number_of_irq(const struct dt_device_node 
> *device)
>       const struct dt_device_node *p;
>       const __be32 *intspec, *tmp;
>       u32 intsize, intlen;
> +    int intnum;
>   
>       dt_dprintk("dt_irq_number: dev=%s\n", device->full_name);
>   
> +    /* Try the new-style interrupts-extended first */
> +    intnum = dt_count_phandle_with_args(device, "interrupts-extended",
> +                                        "#interrupt-cells");
> +    if ( intnum > 0 )
IIUC dt_count_phandle_with_args, 0 would means the property is present but 
doesn't contain any interrupts. I do agree this is a probably a wrong 
device-tree, but technically I am not sure we should try to look for 
"#interrupts" if intnum = 0.
> +    {
> +        dt_dprintk(" intnum=%d\n", intnum);
You are re-using the exact same debug message as for "interrupts". So it would 
be difficult for a developer to know exactly which path is used. Could we print 
message regarding whether "interrupts-extended" or "interrupts" is used?
> +        return intnum;
> +    }
> +
>       /* Get the interrupts property */
>       intspec = dt_get_property(device, "interrupts", &intlen);
>       if ( intspec == NULL )
> @@ -1420,10 +1430,29 @@ int dt_device_get_raw_irq(const struct dt_device_node 
> *device,
>       const __be32 *intspec, *tmp, *addr;
>       u32 intsize, intlen;
>       int res = -EINVAL;
> +    struct dt_phandle_args args;
> +    int i;
>   
>       dt_dprintk("dt_device_get_raw_irq: dev=%s, index=%u\n",
>                  device->full_name, index);
>   
> +    /* Get the reg property (if any) */
> +    addr = dt_get_property(device, "reg", NULL);
> +
> +    /* Try the new-style interrupts-extended first */
> +    res = dt_parse_phandle_with_args(device, "interrupts-extended",
> +                                     "#interrupt-cells", index, &args);
> +    if ( !res )
I don't think the check is correct. dt_parse_phandle_with_args may return a 
negative value in case of an error. So we likely want "res >= 0" here.
> +    {
> +        dt_dprintk(" intspec=%d intsize=%d\n", args.args[0], args.args_count);
Same remark for the message here.

> +
> +        for ( i = 0; i < args.args_count; i++ )
> +            args.args[i] = cpu_to_be32(args.args[i]);
> +
> +        return dt_irq_map_raw(args.np, args.args, args.args_count,
> +                              addr, out_irq);
> +    }
> +
>       /* Get the interrupts property */
>       intspec = dt_get_property(device, "interrupts", &intlen);
>       if ( intspec == NULL )
> @@ -1432,9 +1461,6 @@ int dt_device_get_raw_irq(const struct dt_device_node 
> *device,
>   
>       dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen);
>   
> -    /* Get the reg property (if any) */
> -    addr = dt_get_property(device, "reg", NULL);
> -
>       /* Look for the interrupt parent. */
>       p = dt_irq_find_parent(device);
>       if ( p == NULL )
Cheers,

[1] linux/Documentation/devicetree/bindings/interrupt-controller

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel