Re: [PATCH v4 18/18] xen/arm: IRQ: Handle multiple action per IRQ

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.04.14 at 14:58, <julien.grall@linaro.org> wrote:
> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> interrupt.
> 
> To be able to use multiple action, the driver has to explicitly call
> {setup,request}_irq with IRQF_SHARED as 2nd parameter.
> 
> The behavior stays the same on x86, e.g only one action is handled.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

For the non-ARM part:
Acked-by: Jan Beulich <jbeulich@suse.com>

> ---
>     Changes in v4:
>         - Go back to a single custom linked list. The double linked-list
>         doesn't fit the requirements (i.e browsing safely without look) and
>         the llist.h from Linux doesn't allow use to delete a node in the 
> middle
>         of the list.
> 
>     Changes in v3:
>         - Drop {setup,request}_irq_flags, the current function has been
>         extended on an earlier patch.
>         - Rename IRQ_SHARED into IRQF_SHARED
> 
>     Changes in v2:
>         - Explain design choice
>         - Introduce CONFIG_IRQ_HAS_MULTIPLE_ACTION
>         - Use list instead of custom pointer
>         - release_irq should not shutdown the IRQ at the beginning
>         - Add IRQ_SHARED flags
>         - Introduce request_irq_flags and setup_irq_flags
>         - Fix compilation on x86. Some code are creating the irqaction
>         via = { ... } so the "next" field should be moved
>         toward the end
> ---
>  xen/arch/arm/irq.c           |   79 +++++++++++++++++++++++++++++++++---------
>  xen/include/asm-arm/config.h |    2 ++
>  xen/include/xen/irq.h        |    4 +++
>  3 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 1bc960d..d05c0be 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -155,7 +155,6 @@ int request_irq(unsigned int irq, unsigned int irqflags,
>  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>  {
>      struct irq_desc *desc = irq_to_desc(irq);
> -    struct irqaction *action = desc->action;
>  
>      /* TODO: perfc_incr(irqs); */
>  
> @@ -166,7 +165,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, 
> int is_fiq)
>      spin_lock(&desc->lock);
>      desc->handler->ack(desc);
>  
> -    if ( action == NULL )
> +    if ( !desc->action )
>      {
>          printk("Unknown %s %#3.3x\n",
>                 is_fiq ? "FIQ" : "IRQ", irq);
> @@ -198,12 +197,21 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int 
> irq, int is_fiq)
>  
>      desc->status |= IRQ_INPROGRESS;
>  
> -    action = desc->action;
>      while ( desc->status & IRQ_PENDING )
>      {
> +        struct irqaction *action;
> +
>          desc->status &= ~IRQ_PENDING;
> +        action = desc->action;
> +
>          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);
>      }
>  
> @@ -220,34 +228,71 @@ void release_irq(unsigned int 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);
>  
>      spin_lock_irqsave(&desc->lock,flags);
>  
> -    desc->handler->shutdown(desc);
> +    action_ptr = &desc->action;
> +    for ( ;; )
> +    {
> +        action = *action_ptr;
> +        if ( !action )
> +        {
> +            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", 
> irq);
> +            spin_unlock_irqrestore(&desc->lock, flags);
> +            return;
> +        }
> +
> +        if ( action->dev_id == dev_id )
> +            break;
> +
> +        action_ptr = &action->next;
> +    }
> +
> +    /* Found it - remove it from the action list */
> +    *action_ptr = action->next;
>  
> -    action = desc->action;
> -    desc->action  = NULL;
> -    desc->status &= ~IRQ_GUEST;
> +    /* If this was the last 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->free_on_release )
>          xfree(action);
>  }
>  
> -static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
> +static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
> +                       struct irqaction *new)
>  {
> -    if ( desc->action != NULL )
> -        return -EBUSY;
> +    bool_t shared = !!(irqflags & IRQF_SHARED);
> +
> +    ASSERT(new != NULL);
> +
> +    /* Sanity checks:
> +     *  - if the IRQ is marked as shared
> +     *  - dev_id is not NULL when IRQF_SHARED is set
> +     */
> +    if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) )
> +        return -EINVAL;
> +    if ( shared && new->dev_id == NULL )
> +        return -EINVAL;
> +
> +    if ( shared )
> +        desc->status |= IRQF_SHARED;
>  
> -    desc->action  = new;
> -    dsb(sy);
> +    new->next = desc->action;
> +    dsb(ish);
> +    desc->action = new;
> +    dsb(ish);
>  
>      return 0;
>  }
> @@ -275,7 +320,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, 
> struct irqaction *new)
>  
>      disabled = (desc->action == NULL);
>  
> -    rc = __setup_irq(desc, new);
> +    rc = __setup_irq(desc, irqflags, new);
>      if ( rc )
>          goto err;
>  
> @@ -340,7 +385,7 @@ int route_dt_irq_to_guest(struct domain *d, const struct 
> dt_irq *irq,
>          goto out;
>      }
>  
> -    retval = __setup_irq(desc, action);
> +    retval = __setup_irq(desc, 0, action);
>      if ( retval )
>          goto out;
>  
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index ef291ff..1c3abcf 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -37,6 +37,8 @@
>  
>  #define CONFIG_VIDEO 1
>  
> +#define CONFIG_IRQ_HAS_MULTIPLE_ACTION 1
> +
>  #define OPT_CONSOLE_STR "dtuart"
>  
>  #ifdef MAX_PHYS_CPUS
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index f9a18d8..40c0f3f 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -14,6 +14,9 @@ struct irqaction {
>      const char *name;
>      void *dev_id;
>      bool_t free_on_release;
> +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> +    struct irqaction *next;
> +#endif
>  };
>  
>  /*
> @@ -27,6 +30,7 @@ struct irqaction {
>  #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
>  #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
>  #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest 
> EOI */
> +#define IRQF_SHARED       (1<<8)  /* IRQ is shared */
>  
>  /* Special IRQ numbers. */
>  #define AUTO_ASSIGN_IRQ         (-1)
> -- 
> 1.7.10.4