From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 1/2] xen/arm: save/restore irq flags in gic_disable_cpu Date: Tue, 16 Apr 2013 11:39:22 +0100 Message-ID: <516D2A5A.9020204@citrix.com> References: <1366044655-12353-1-git-send-email-julien.grall@citrix.com> <1366044655-12353-2-git-send-email-julien.grall@citrix.com> <1366103569.8399.125.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1366103569.8399.125.camel@zakaz.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: Stefano Stabellini , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 04/16/2013 10:12 AM, Ian Campbell wrote: > On Mon, 2013-04-15 at 17:50 +0100, Julien Grall wrote: >> On SMP board, gic_disable_cpu is called by an SGI call function when Xen is >> halting. In this specific case, the interrupts are disabled. > > In the only other existing caller (__cpu_disable) interrupts are also > disabled and I'd argue that it would be invalid to call this function > with interrupts enabled due to its nature. I think it should start with > an assert to that effect. Indeed, I didn't pay attention that interrupts are disabled in __cpu_disable. I will use spin_lock and add an assert. >> Signed-off-by: Julien Grall >> --- >> xen/arch/arm/gic.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index aa179a9..c9f64f1 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -419,10 +419,12 @@ void __cpuinit gic_init_secondary_cpu(void) >> /* Shut down the per-CPU GIC interface */ >> void gic_disable_cpu(void) >> { >> - spin_lock_irq(&gic.lock); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&gic.lock, flags); >> gic_cpu_disable(); > > So we apparently have gic_cpu_disable and gic_disable_cpu, nice! > > Since the only caller of gic_cpu_disable is gic_disable_cpu I think we > should just inline it. Or call it gic_(cpu_)disable_gicc > >> gic_hyp_disable(); > > Likewise this is the only caller of this function, either inline or > gic_(cpu)disable_gich? If we modify the name, we also need to modify gic_{hyp,cpu,dist}_init. I would prefer to let in state or rename all the functions. It's clearer to have separate functions (init and disable) for each part of the gic. Julien