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

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

On 04/16/2014 04:54 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>>          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);
> 
> You aren't removing entries from within the loop so I don't think you
> need the _safe variant.

As we release the desc->lock here, it might be possible to have the list
changed under the CPU feet by release_irq.

With the double-linked list, how do we make sure that it won't happen?

>>          spin_lock_irq(&desc->lock);
>>      }
>>  
>> @@ -217,33 +220,69 @@ 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;
> 
> Extra space before =.
> 
> I actually think a goto found would actually be clearer here than the
> flag.

I'm not a big fan of goto.

Anyway, I will use it here if you think it's clearer.

> 	for each
> 		if (got it)
> 			goto found
> 
> 	printk(not found)
> 	return
> 
>   found:
> 	/* Found it. */
>> +    /* Sanity checks:
>> +     *  - if the IRQ is marked as shared
>> +     *  - dev_id is not NULL when IRQF_SHARED is set
>> +     */
>> +    if ( !list_empty(&desc->action) &&
>> +         (!(desc->status & IRQF_SHARED) || !shared) )
>> +        return -EINVAL;
> 
> Did you mean EBUSY?

Right, EBUSY would be better here.

>> @@ -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 */
> 
> This was never actually a list I think, and the comment is certainly
> wrong now.

I guess it was copied from Linux :). I will change the comment into
"IRQ action"

Regards,

-- 
Julien Grall