From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ Date: Wed, 16 Apr 2014 17:06:04 +0100 Message-ID: <534EAA6C.9070601@linaro.org> References: <1396968247-8768-1-git-send-email-julien.grall@linaro.org> <1396968247-8768-19-git-send-email-julien.grall@linaro.org> <1397663693.24638.226.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WaSLg-0006pi-21 for xen-devel@lists.xenproject.org; Wed, 16 Apr 2014 16:06:08 +0000 Received: by mail-ee0-f43.google.com with SMTP id e53so8985289eek.16 for ; Wed, 16 Apr 2014 09:06:06 -0700 (PDT) In-Reply-To: <1397663693.24638.226.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, Keir Fraser , tim@xen.org, Jan Beulich , stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.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