Re: [PATCH v2 10/16] xen/arm: IRQ: Don't need to have a specific function to route IRQ to Xen

[Julien Grall <julien.grall@linaro.org>]

On 04/07/2014 02:53 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
>> When the IRQ is handling by Xen, the setup is done in 2 steps:
> 
> "an IRQ is handled" (and perhaps s/, the//)

Will fix it.

> $subject is an odd way to describe the change too (it's more like the
> motivation). Something like "defer routing IRQ to Xen until setup_irq()
> call" perhaps?

Sounds better. I will change the commit title.

> 
>>     - Route the IRQ to the current CPU and set priorities
>>     - Set up the handler
>>
>> For PPIs, these steps are called on every cpu. For SPIs, they are only called
>> on the boot CPU.
>>
>> Dividing the setup in two step complicates the code when a new driver is
>> added to Xen (for instance a SMMU driver). Xen can safely route the IRQ
>> when the driver sets up the interrupt handler.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Fix typo in commit message
>>         - s/SGI/SPI/ in comments
>>         - Rename gic_route_dt_irq into gic_route_irq_to_xen which is
>>         taking a desc now
>>         - Call setup_irq before initializing the GIC IRQ as the first one
>>         can fail.
>> ---
>>  xen/arch/arm/gic.c         |   63 +++++++-------------------------------------
>>  xen/arch/arm/irq.c         |   24 ++++++++++++++++-
>>  xen/arch/arm/setup.c       |    2 --
>>  xen/arch/arm/smpboot.c     |    2 --
>>  xen/arch/arm/time.c        |   11 --------
>>  xen/include/asm-arm/gic.h  |   10 +++----
>>  xen/include/asm-arm/time.h |    3 ---
>>  7 files changed, 36 insertions(+), 79 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 8c53e52..9127ecf 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -257,30 +257,20 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>      spin_unlock(&gic.lock);
>>  }
>>  
>> -/* Program the GIC to route an interrupt */
>> -static int gic_route_irq(unsigned int irq, bool_t level,
>> -                         const cpumask_t *cpu_mask, unsigned int priority)
>> +/* Program the GIC to route an interrupt to the host (eg Xen)
> 
> You mean i.e. not e.g.

Will fix it.

>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 798353b..1262a9c 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -247,15 +247,37 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>      int rc;
>>      unsigned long flags;
>>      struct irq_desc *desc;
>> +    bool_t disabled = 0;
> 
> No need to init, it's unconditionally assigned below. But if you do want
> to keep it then I think boot_t wants to go with false even if that is
> the same as 0 in the end.

Ok.

>>      desc = irq_to_desc(irq->irq);
>>  
>>      spin_lock_irqsave(&desc->lock, flags);
>> +
>> +    disabled = (desc->action == NULL);
>> +
>>      rc = __setup_irq(desc, new);
>> +    if ( rc )
>> +        goto err;
>>  
>> -    if ( !rc )
>> +    /* First time the IRQ is setup */
>> +    if ( disabled )
> 
> There's no way we can get back into this state. Perhaps with calls to
> release_irq?

release_irq will disable the interrupt if all the actions are removed.

> 
>> +    {
>> +        bool_t level;
>> +
>> +        level = dt_irq_is_level_triggered(irq);
>> +        /* It's fine to use smp_processor_id() because:
>> +         * For PPI: irq_desc is banked
>> +         * For SPI: we don't care for now which CPU will receive the
>> +         * interrupt
> 
> setup_dt_irq expected to be called multiple times for a PPI and the desc
> is not shared, so that's how they get setup as well, right?

Yes, this setup_dt_irq should be called on the right processor when the
IRQ is a PPI.

Regards,

-- 
Julien Grall