From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 10/16] xen/arm: IRQ: Don't need to have a specific function to route IRQ to Xen Date: Mon, 07 Apr 2014 15:15:37 +0100 Message-ID: <5342B309.20006@linaro.org> References: <1396557727-19102-1-git-send-email-julien.grall@linaro.org> <1396557727-19102-11-git-send-email-julien.grall@linaro.org> <1396878824.22845.105.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 1WXAKp-0006cN-Ul for xen-devel@lists.xenproject.org; Mon, 07 Apr 2014 14:15:40 +0000 Received: by mail-ee0-f51.google.com with SMTP id c13so699461eek.10 for ; Mon, 07 Apr 2014 07:15:38 -0700 (PDT) In-Reply-To: <1396878824.22845.105.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:53 PM, Ian Campbell wrote: > On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: >> When the IRQ is handling by Xen, the setup is done in 2 steps: > > "an IRQ is handled" (and perhaps s/, the//) Will fix it. > $subject is an odd way to describe the change too (it's more like the > motivation). Something like "defer routing IRQ to Xen until setup_irq() > call" perhaps? Sounds better. I will change the commit title. > >> - Route the IRQ to the current CPU and set priorities >> - Set up the handler >> >> For PPIs, these steps are called on every cpu. For SPIs, they are only called >> on the boot CPU. >> >> Dividing the setup in two step complicates the code when a new driver is >> added to Xen (for instance a SMMU driver). Xen can safely route the IRQ >> when the driver sets up the interrupt handler. >> >> Signed-off-by: Julien Grall >> >> --- >> Changes in v2: >> - Fix typo in commit message >> - s/SGI/SPI/ in comments >> - Rename gic_route_dt_irq into gic_route_irq_to_xen which is >> taking a desc now >> - Call setup_irq before initializing the GIC IRQ as the first one >> can fail. >> --- >> xen/arch/arm/gic.c | 63 +++++++------------------------------------- >> xen/arch/arm/irq.c | 24 ++++++++++++++++- >> xen/arch/arm/setup.c | 2 -- >> xen/arch/arm/smpboot.c | 2 -- >> xen/arch/arm/time.c | 11 -------- >> xen/include/asm-arm/gic.h | 10 +++---- >> xen/include/asm-arm/time.h | 3 --- >> 7 files changed, 36 insertions(+), 79 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 8c53e52..9127ecf 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -257,30 +257,20 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, >> spin_unlock(&gic.lock); >> } >> >> -/* Program the GIC to route an interrupt */ >> -static int gic_route_irq(unsigned int irq, bool_t level, >> - const cpumask_t *cpu_mask, unsigned int priority) >> +/* Program the GIC to route an interrupt to the host (eg Xen) > > You mean i.e. not e.g. Will fix it. >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 798353b..1262a9c 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -247,15 +247,37 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >> int rc; >> unsigned long flags; >> struct irq_desc *desc; >> + bool_t disabled = 0; > > No need to init, it's unconditionally assigned below. But if you do want > to keep it then I think boot_t wants to go with false even if that is > the same as 0 in the end. Ok. >> desc = irq_to_desc(irq->irq); >> >> spin_lock_irqsave(&desc->lock, flags); >> + >> + disabled = (desc->action == NULL); >> + >> rc = __setup_irq(desc, new); >> + if ( rc ) >> + goto err; >> >> - if ( !rc ) >> + /* First time the IRQ is setup */ >> + if ( disabled ) > > There's no way we can get back into this state. Perhaps with calls to > release_irq? release_irq will disable the interrupt if all the actions are removed. > >> + { >> + bool_t level; >> + >> + level = dt_irq_is_level_triggered(irq); >> + /* It's fine to use smp_processor_id() because: >> + * For PPI: irq_desc is banked >> + * For SPI: we don't care for now which CPU will receive the >> + * interrupt > > setup_dt_irq expected to be called multiple times for a PPI and the desc > is not shared, so that's how they get setup as well, right? Yes, this setup_dt_irq should be called on the right processor when the IRQ is a PPI. Regards, -- Julien Grall