From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 09/16] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown} Date: Mon, 07 Apr 2014 15:45:45 +0100 Message-ID: <5342BA19.8020600@linaro.org> References: <1396557727-19102-1-git-send-email-julien.grall@linaro.org> <1396557727-19102-10-git-send-email-julien.grall@linaro.org> <1396877257.22845.92.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.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WXAo0-00009D-TU for xen-devel@lists.xenproject.org; Mon, 07 Apr 2014 14:45:49 +0000 Received: by mail-ee0-f45.google.com with SMTP id d17so721032eek.32 for ; Mon, 07 Apr 2014 07:45:46 -0700 (PDT) In-Reply-To: <1396877257.22845.92.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, tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On 04/07/2014 02:27 PM, Ian Campbell wrote: > On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: > > In the subject "constraint". > > A better title might be "require $FOO lock be held by callers of > gic_irq...blah" I will change the title. >> When multiple action will be supported, gic_irq_{startup,shutdown} will have > > s/will be/are/ > >> to be called in the same critical zone as setup/release. > > "critical section" is the more usual term I think. Or "under the same > lock as". > >> Otherwise it could have a race condition if at the same time CPU A is calling > > "Otherwise there is a race condition..." > >> release_dt_irq and CPU B is calling setup_dt_irq. >> >> This could end up to the IRQ not being enabled. > > s/to/with/ I will fix all the typoes. >> Signed-off-by: Julien Grall >> >> --- >> Changes in v2: >> - Fix typoes in commit message >> - Move this patch earlier in the series => move shutdown() in >> release_irq and gic_route_irq >> --- >> xen/arch/arm/gic.c | 39 ++++++++++++++++++++++++--------------- >> xen/arch/arm/irq.c | 6 ++++-- >> 2 files changed, 28 insertions(+), 17 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 82e0316..8c53e52 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -123,44 +123,53 @@ void gic_restore_state(struct vcpu *v) >> gic_restore_pending_irqs(v); >> } >> >> -static void gic_irq_enable(struct irq_desc *desc) >> +static unsigned int gic_irq_startup(struct irq_desc *desc) > > Is there code motion mixed in with this locking change? > > It looks a bit like the relationship between e.g. gic_irq_startup and > gic_irq_enable is being turned inside out? Maybe diff has just chosen an > unhelpful representation of a relatively simple change? [..] > Since desc->handler is a generic construct I think it is worth > mentioning in the commit log that this is consistent with x86. > > After this change are arm's locking requirements wrt the > hw_irq_controller callbacks now consistent with x86's? Before this patch gic_irq_startup was calling gic_irq_enable, now it's flipped. After thinking, I will rework this patch. It's also possible to take the desc->lock outside for irq_enable and irq_startup. So we will be consistent with x86's that AFAIU request desc->lock to be held by the callers. Regards, -- Julien Grall