diff mbox

[Xen-devel,v2,16/16] xen/arm: IRQ: Handle multiple action per IRQ

Message ID 1396557727-19102-17-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall April 3, 2014, 8:42 p.m. UTC 
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_flags (newly created) with IRQ_SHARED as 3rd parameter.

The behavior stays the same on x86, e.g only one action is handled.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>

---
    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           |  106 +++++++++++++++++++++++++++++++-----------
 xen/common/irq.c             |    3 ++
 xen/include/asm-arm/config.h |    2 +
 xen/include/asm-arm/irq.h    |    7 +++
 xen/include/xen/irq.h        |    8 ++++
 5 files changed, 99 insertions(+), 27 deletions(-)

Comments

Jan Beulich April 4, 2014, 7:59 a.m. UTC | #1 
>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>  unsigned int platform_get_irq(const struct dt_device_node *device,
>                                int index);
>  
> +int request_irq_flags(unsigned int irq,
> +                      void (*handler)(int, void *, struct cpu_user_regs *),
> +                      unsigned int irqflags,
> +                      const char * devname, void *dev_id);
> +int setup_irq_flags(unsigned int irq, struct irqaction *new,
> +                    unsigned int irqflags);
> +

I think it is a bad choice to have separate ..._flags() versions of these,
the normal functions should take flags at least when
CONFIG_IRQ_HAS_MULTIPLE_ACTION. Perhaps, with the expectation
of wanting to add further flags in the future, it would be reasonable to
have the functions always have the flags parameters, and assert the
value passed is zero on x86.

> @@ -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 IRQ_SHARED        (1<<8)  /* IRQ is shared */

This is now given two meanings (input to above functions and status),
which in the long run can become problematic. Please follow Linux by
using two distinct flag sets.

Jan
Julien Grall April 4, 2014, 8:52 a.m. UTC | #2 
On 04/04/2014 08:59 AM, Jan Beulich wrote:
>>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>  unsigned int platform_get_irq(const struct dt_device_node *device,
>>                                int index);
>>  
>> +int request_irq_flags(unsigned int irq,
>> +                      void (*handler)(int, void *, struct cpu_user_regs *),
>> +                      unsigned int irqflags,
>> +                      const char * devname, void *dev_id);
>> +int setup_irq_flags(unsigned int irq, struct irqaction *new,
>> +                    unsigned int irqflags);
>> +
> 
> I think it is a bad choice to have separate ..._flags() versions of these,
> the normal functions should take flags at least when
> CONFIG_IRQ_HAS_MULTIPLE_ACTION. Perhaps, with the expectation
> of wanting to add further flags in the future, it would be reasonable to
> have the functions always have the flags parameters, and assert the
> value passed is zero on x86.

I chose this solution because I didn't know if extending the prototype
{request,setup}_irq would be acceptable.

With this patch series I've tried to remove ARM specific functions (e.g
{request,setup}_dt_irq) for IRQ management. So I would prefer to extend
the both function prototypes unconditionally to avoid ifdery in serial
drivers.

> 
>> @@ -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 IRQ_SHARED        (1<<8)  /* IRQ is shared */
> 
> This is now given two meanings (input to above functions and status),
> which in the long run can become problematic. Please follow Linux by
> using two distinct flag sets.

Ok, I will rename it Ito IRQF_SHARED and introduce a new field "flags"
in irq_desc.

Regards,
Jan Beulich April 4, 2014, 9 a.m. UTC | #3 
>>> On 04.04.14 at 10:52, <julien.grall@linaro.org> wrote:
> On 04/04/2014 08:59 AM, Jan Beulich wrote:
>>>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
>>> @@ -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 IRQ_SHARED        (1<<8)  /* IRQ is shared */
>> 
>> This is now given two meanings (input to above functions and status),
>> which in the long run can become problematic. Please follow Linux by
>> using two distinct flag sets.
> 
> Ok, I will rename it Ito IRQF_SHARED and introduce a new field "flags"
> in irq_desc.

No, sorry, that's not what I meant. I want the new definition solely
for the purpose of passing as function argument. Folding all the flags
internally into the status field is quite okay for the time being.

Jan
diff mbox

Patch

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index ef67279..556c2e1 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -68,7 +68,6 @@  static int __init init_irq_data(void)
         struct irq_desc *desc = irq_to_desc(irq);
         init_one_irq_desc(desc);
         desc->irq = irq;
-        desc->action  = NULL;
     }
 
     return 0;
@@ -82,7 +81,6 @@  static int __cpuinit init_local_irq_data(void)
         struct irq_desc *desc = irq_to_desc(irq);
         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
          * CPU0 otherwise we may miss the type if the IRQ type has been
@@ -107,16 +105,21 @@  void __cpuinit init_secondary_IRQ(void)
 
 static inline struct domain *irq_get_domain(struct irq_desc *desc)
 {
+    struct irqaction *action;
+
     ASSERT(spin_is_locked(&desc->lock));
     ASSERT(desc->status & IRQ_GUEST);
-    ASSERT(desc->action != NULL);
+    ASSERT(!list_empty(&desc->action));
+
+    action = list_entry(desc->action.next, struct irqaction, next);
 
-    return desc->action->dev_id;
+    return action->dev_id;
 }
 
-int request_irq(unsigned int irq,
-                void (*handler)(int, void *, struct cpu_user_regs *),
-                const char *devname, void *dev_id)
+int request_irq_flags(unsigned int irq,
+                      void (*handler)(int, void *, struct cpu_user_regs *),
+                      unsigned int irqflags,
+                      const char *devname, void *dev_id)
 {
     struct irqaction *action;
     int retval;
@@ -141,18 +144,24 @@  int request_irq(unsigned int irq,
     action->dev_id = dev_id;
     action->free_on_release = 1;
 
-    retval = setup_irq(irq, action);
+    retval = setup_irq_flags(irq, action, irqflags);
     if ( retval )
         xfree(action);
 
     return retval;
 }
 
+int request_irq(unsigned int irq,
+                void (*handler)(int, void *, struct cpu_user_regs *),
+                const char *devname, void *dev_id)
+{
+    return request_irq_flags(irq, handler, 0, devname, dev_id);
+}
+
 /* Dispatch an interrupt */
 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); */
 
@@ -163,7 +172,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 ( list_empty(&desc->action) )
     {
         printk("Unknown %s %#3.3x\n",
                is_fiq ? "FIQ" : "IRQ", irq);
@@ -195,12 +204,14 @@  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, *next;
+
         desc->status &= ~IRQ_PENDING;
         spin_unlock_irq(&desc->lock);
-        action->handler(irq, action->dev_id, regs);
+        list_for_each_entry_safe(action, next, &desc->action, next)
+            action->handler(irq, action->dev_id, regs);
         spin_lock_irq(&desc->lock);
     }
 
@@ -217,39 +228,75 @@  void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
-   struct irqaction *action;
+    struct irqaction *action;
+    bool_t found = 0;
 
     desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock,flags);
 
-    desc->handler->shutdown(desc);
+    list_for_each_entry(action, &desc->action, next)
+    {
+        if ( action->dev_id == dev_id )
+        {
+            found  = 1;
+            break;
+         }
+    }
 
-    action = desc->action;
-    desc->action  = NULL;
-    desc->status &= ~IRQ_GUEST;
+    if ( !found )
+    {
+        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
+        return;
+    }
+
+    /* Found it - remove it from the action list */
+    list_del_init(&action->next);
+
+    /* If this was the last action, shut down the IRQ */
+    if ( list_empty(&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, struct irqaction *new,
+                       unsigned int irqflags)
 {
-    if ( desc->action != NULL )
-        return -EBUSY;
+    bool_t shared = !!(irqflags & IRQ_SHARED);
+
+    ASSERT(new != NULL);
 
-    desc->action  = new;
+    /* Sanity checks:
+     *  - if the IRQ is marked as shared
+     *  - dev_id is not NULL when IRQ_SHARED is set
+     */
+    if ( !list_empty(&desc->action) && (!(desc->status & IRQ_SHARED) || !shared) )
+        return -EINVAL;
+    if ( shared && new->dev_id == NULL )
+        return -EINVAL;
+
+    if ( shared )
+        desc->status |= IRQ_SHARED;
+
+    INIT_LIST_HEAD(&new->next);
+    list_add_tail(&new->next, &desc->action);
     dsb(sy);
 
     return 0;
 }
 
-int setup_irq(unsigned int irq, struct irqaction *new)
+int setup_irq_flags(unsigned int irq, struct irqaction *new,
+                    unsigned int irqflags)
 {
     int rc;
     unsigned long flags;
@@ -270,9 +317,9 @@  int setup_irq(unsigned int irq, struct irqaction *new)
         return -EBUSY;
     }
 
-    disabled = (desc->action == NULL);
+    disabled = list_empty(&desc->action);
 
-    rc = __setup_irq(desc, new);
+    rc = __setup_irq(desc, new, irqflags);
     if ( rc )
         goto err;
 
@@ -297,6 +344,11 @@  err:
     return rc;
 }
 
+int setup_irq(unsigned int irq, struct irqaction *new)
+{
+    return setup_irq_flags(irq, new, 0);
+}
+
 int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                           const char * devname)
 {
@@ -320,7 +372,7 @@  int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
      *  - Otherwise -> For now, don't allow the IRQ to be shared between
      *  Xen and domains.
      */
-    if ( desc->action != NULL )
+    if ( !list_empty(&desc->action) )
     {
         struct domain *ad = irq_get_domain(desc);
 
@@ -337,7 +389,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, action, 0);
     if ( retval )
     {
         xfree(action);
diff --git a/xen/common/irq.c b/xen/common/irq.c
index 3e55dfa..688e48a 100644
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -17,6 +17,9 @@  int init_one_irq_desc(struct irq_desc *desc)
     spin_lock_init(&desc->lock);
     cpumask_setall(desc->affinity);
     INIT_LIST_HEAD(&desc->rl_link);
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+    INIT_LIST_HEAD(&desc->action);
+#endif
 
     err = arch_init_one_irq_desc(desc);
     if ( err )
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 5b7b1a8..079e8b9 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/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index ad2cc18..80dd452 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -45,6 +45,13 @@  int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
 unsigned int platform_get_irq(const struct dt_device_node *device,
                               int index);
 
+int request_irq_flags(unsigned int irq,
+                      void (*handler)(int, void *, struct cpu_user_regs *),
+                      unsigned int irqflags,
+                      const char * devname, void *dev_id);
+int setup_irq_flags(unsigned int irq, struct irqaction *new,
+                    unsigned int irqflags);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 1f8bdb3..49a805b 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 list_head 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 IRQ_SHARED        (1<<8)  /* IRQ is shared */
 
 /* Special IRQ numbers. */
 #define AUTO_ASSIGN_IRQ         (-1)
@@ -68,7 +72,11 @@  typedef struct irq_desc {
     unsigned int status;        /* IRQ status */
     hw_irq_controller *handler;
     struct msi_desc   *msi_desc;
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+    struct list_head action;    /* IRQ action list */
+#else
     struct irqaction *action;   /* IRQ action list */
+#endif
     int irq;
     spinlock_t lock;
     struct arch_irq_desc arch;