Re: [Xen-devel] [PATCH v4 14/18] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-22 at 13:58 +0100, Julien Grall wrote:
> For now, ARM uses different IRQ functions to setup an interrupt handler. This
> is a bit annoying for common driver because we have to add idefery when
> an IRQ is setup (see ns16550_init_postirq for an example).
> 
> To avoid to completely fork the IRQ management code, we can introduce a field
> to store the IRQ type (e.g level/edge ...).
> 
> This patch also adds platform_get_irq which will retrieve the IRQ from the
> device tree and setup correctly the IRQ type.
> 
> In order to use this solution, we have to move init_IRQ earlier for the boot
> CPU. It's fine because the code only depends on percpu.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>

Mostly just grammar nits.

> @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void)
>          init_one_irq_desc(desc);
>          desc->irq = irq;
>          desc->action  = NULL;
> +
> +        /* PPIs are include in local_irqs, we have to copy the IRQ type from

"included"

> +         * CPU0 otherwise we may miss the type if the IRQ type has been
> +         * set early.

what does "set early" mean, how early is early? Initialised before
secondary CPUs were brought up perhaps?

        /* PPIs are included in local_irqs, we copy the IRQ type from
         * CPU0 when bringing up secondary cpus in order to pick up any
         * configuration done before this CPU came up. For interrupts
         * configured after this point this is done in XXX().

Why is the copying not conditional on CPU!=0?

> +    /* Setup the IRQ type */
> +    if ( irq < NR_LOCAL_IRQS )
> +    {
> +        unsigned int cpu;
> +        /* For PPIs, we need to set IRQ type on every online CPUs */

CPU should be singular here.

> +        for_each_cpu( cpu, &cpu_online_map )
> +        {
> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
> +            res = irq_set_type(desc, type);
> +            if ( res )
> +            {
> +                /*
> +                 * For PPIs the IRQ type is the same on every CPU. It
> +                 * should not fail to other CPU than CPU0

"It should not fail on other CPUs than CPU0" (or "cannot fail" would be
stronger, and an assert is pretty strong).

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel