Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> interrupt.

Mention here that you are therefore creating a linked list of actions
for each interrupt.

If you use xen/list.h for this then you get a load of helpers and
iterators which would save you open coding them.

Some discussion of the behaviour wrt acking the physical interrupt might
also be interesting, especially in the case where the shared IRQ is
routed to the guest and to the hypervisor or to multiple guests.

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: Keir Fraser <keir@xen.org>
> ---
>  xen/arch/arm/gic.c    |   48 ++++++++++++++++++++++++++++++++++++++++--------
>  xen/arch/arm/irq.c    |    6 +++++-
>  xen/include/xen/irq.h |    1 +
>  3 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ebd2b5f..8ba1de3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -534,32 +534,64 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
>  {
>      struct irq_desc *desc;
>      unsigned long flags;
> -   struct irqaction *action;
> +    struct irqaction *action, **action_ptr;
>  
>      desc = irq_to_desc(irq->irq);
>  
>      spin_lock_irqsave(&desc->lock,flags);
>      desc->handler->shutdown(desc);
>      action = desc->action;
> -    desc->action  = NULL;
> -    desc->status &= ~IRQ_GUEST;
> +
> +    action_ptr = &desc->action;
> +    for ( ;; )
> +    {
> +        action = *action_ptr;
> +
> +        if ( !action )
> +        {
> +            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n",
> +                   irq->irq);
> +            return;
> +        }
> +
> +        if ( action->dev_id == dev_id )
> +            break;
> +
> +        action_ptr = &action->next;
> +    }
> +
> +    /* Found it - remove it from the action list */
> +    *action_ptr = action->next;
> +
> +    /* If this was the list action, shut down the IRQ */
> +    if ( !desc->action )
> +    {
> +        desc->handler->shutdown(desc);
> +        desc->status &= ~IRQ_GUEST;
> +    }
>  
>      spin_unlock_irqrestore(&desc->lock,flags);
>  
>      /* Wait to make sure it's not being used on another CPU */
>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
>  
> -    if (action && action->free_on_release)
> +    if ( action && action->free_on_release )
>          xfree(action);
>  }
>  
>  static int __setup_irq(struct irq_desc *desc, unsigned int irq,
>                         struct irqaction *new)
>  {
> -    if ( desc->action != NULL )
> -        return -EBUSY;
> +    struct irqaction *action = desc->action;
> +
> +    ASSERT(new != NULL);
> +
> +    /* Check that dev_id is correctly filled if we have multiple action */
> +    if ( action != NULL && ( action->dev_id == NULL || new->dev_id == NULL ) )
> +        return -EINVAL;
>  
> -    desc->action  = new;
> +    new->next = desc->action;
> +    desc->action = new;
>      dsb();
>  
>      return 0;
> @@ -610,7 +642,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  
>      rc = __setup_irq(desc, irq->irq, new);
>  
> -    if ( !rc )
> +    if ( !rc && disabled )
>          desc->handler->startup(desc);
>  
>      spin_unlock_irqrestore(&desc->lock, flags);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 3e326b0..edf0404 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -179,7 +179,11 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>      {
>          desc->status &= ~IRQ_PENDING;
>          spin_unlock_irq(&desc->lock);
> -        action->handler(irq, action->dev_id, regs);
> +        do
> +        {
> +            action->handler(irq, action->dev_id, regs);
> +            action = action->next;
> +        } while ( action );
>          spin_lock_irq(&desc->lock);
>      }
>  
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index f2e6215..54314b8 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -11,6 +11,7 @@
>  
>  struct irqaction {
>      void (*handler)(int, void *, struct cpu_user_regs *);
> +    struct irqaction *next;
>      const char *name;
>      void *dev_id;
>      bool_t free_on_release;