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

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

On 03/31/2014 04:53 PM, Ian Campbell wrote:
> On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote:
>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>> 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.
>>
>> I've tried to use xen/list.h. The amount of code it's basically the same
>> and we I have to write open code to get the first element of the list
> 
> Why? Can you post your WIP patch please for comparison.

Because:
	- there is no helper to get the first element (__setup_irq)
	- I need to use 2 variables to search for an element in a list as there is
	no way to know after the end of the loop if we found or not an element.

Below an incremental patch which change next field to a list (doesn't compile
and not finished):

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 7ea4da8..f4f5b71 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -545,36 +545,33 @@ void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
-    struct irqaction *action, **action_ptr;
+    struct irqaction *action, *next;
 
     desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock,flags);
-    desc->handler->shutdown(desc);
-    action = desc->action;
 
-    action_ptr = &desc->action;
-    for ( ;; )
+    action = NULL;
+    list_for_each_entry(next, &desc->action, next)
     {
-        action = *action_ptr;
-
-        if ( !action )
+        if ( next->dev_id == dev_id )
         {
-            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
-            return;
-        }
-
-        if ( action->dev_id == dev_id )
+            action = next;
             break;
+        }
+    }
 
-        action_ptr = &action->next;
+    if ( !action )
+    {
+        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
+        return;
     }
 
     /* Found it - remove it from the action list */
-    *action_ptr = action->next;
+    list_del_init(&action->next);
 
     /* If this was the list action, shut down the IRQ */
-    if ( !desc->action )
+    if ( list_empty(&desc->action) )
     {
         desc->handler->shutdown(desc);
         desc->status &= ~IRQ_GUEST;
@@ -592,12 +589,10 @@ void release_irq(unsigned int irq, const void *dev_id)
 static int __setup_irq(struct irq_desc *desc, struct irqaction *new,
                        unsigned int irqflags)
 {
-    struct irqaction *action = desc->action;
-
     ASSERT(new != NULL);
 
     /* Sanity check if the IRQ already have an action attached */
-    if ( action != NULL )
+    if ( !list_empty(&desc->action) )
     {
         /* Check that IRQ is marked as shared */
         if ( !(desc->status & IRQ_SHARED) || !(irqflags & IRQ_SHARED) )
@@ -610,8 +605,8 @@ static int __setup_irq(struct irq_desc *desc, struct irqaction *new,
     if ( irqflags & IRQ_SHARED )
         desc->status |= IRQ_SHARED;
 
-    new->next = desc->action;
-    desc->action = new;
+    INIT_LIST_HEAD(&new->next);
+    list_add_tail(&new->next, &desc->action);
     dsb(sy);
 
     return 0;
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d6f3500..8a9ae3d 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -56,6 +56,7 @@ irq_desc_t *__irq_to_desc(int irq)
 int __init arch_init_one_irq_desc(struct irq_desc *desc)
 {
     desc->arch.type = DT_IRQ_TYPE_NONE;
+    INIT_LIST_HEAD(&desc->action);
     return 0;
 }
 
@@ -151,7 +152,6 @@ int request_irq(unsigned int irq,
 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); */
 
@@ -162,7 +162,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);
@@ -171,6 +171,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
 
     if ( desc->status & IRQ_GUEST )
     {
+        struct irqaction *action = list_entry(&desc->action, struct irqaction,
+                                              next);
         struct domain *d = action->dev_id;
 
         desc->handler->end(desc);
@@ -194,16 +196,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);
-        do
-        {
+        list_for_each_entry_safe(action, next, &desc->action, next)
             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 20548aa..a926554 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -17,7 +17,9 @@ struct irqaction {
     const char *name;
     void *dev_id;
     bool_t free_on_release;
-    struct irqaction *next;
+#ifdef CONFIG_ARM
+    struct list_head next;
+#endif
 };
 
 /*
@@ -73,7 +75,11 @@ typedef struct irq_desc {
     unsigned int status;        /* IRQ status */
     hw_irq_controller *handler;
     struct msi_desc   *msi_desc;
+#ifdef CONFIG_ARM
+    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;

-- 
Julien Grall