From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH 09/19] xen/arm: Update irq descriptor for LPIs support Date: Mon, 02 Mar 2015 14:17:01 +0000 Message-ID: <54F470DD.3020306@linaro.org> References: <1425299435-3278-1-git-send-email-vijay.kilari@gmail.com> <1425299435-3278-10-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1425299435-3278-10-git-send-email-vijay.kilari@gmail.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: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 02/03/15 12:30, vijay.kilari@gmail.com wrote: > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index cb9c99b..d52ee0c 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -89,6 +89,7 @@ static int __cpuinit init_local_irq_data(void) > init_one_irq_desc(desc); > desc->irq = irq; > desc->action = NULL; > + desc->data = NULL; Why do you initialize desc->data and not desc->virq? > > /* PPIs are included in local_irqs, we copy the IRQ type from > * local_irqs_type when bringing up local IRQ for this CPU in > @@ -104,6 +105,23 @@ static int __cpuinit init_local_irq_data(void) > return 0; > } > > +int irq_set_desc_data(unsigned int irq, void *d) > +{ > + unsigned long flags; > + struct irq_desc *desc = irq_to_desc(irq); > + > + spin_lock_irqsave(&desc->lock, flags); > + desc->data = d; > + spin_unlock_irqrestore(&desc->lock, flags); > + > + return 0; > +} > + > +void *irq_get_desc_data(struct irq_desc *d) > +{ > + return d->data; > +} > + None of those helper are used within this patch series... > void __init init_IRQ(void) > { > int irq; > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index 435dfcd..de029e4 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -46,7 +46,8 @@ void arch_move_irqs(struct vcpu *v); > > /* Set IRQ type for an SPI */ > int irq_set_spi_type(unsigned int spi, unsigned int type); > - There is not much reason to drop this blank line. > +int irq_set_desc_data(unsigned int irq, void *d); > +void *irq_get_desc_data(struct irq_desc *d); > int platform_get_irq(const struct dt_device_node *device, int index); > > void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask); > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h > index 9e0155c..f12afac 100644 > --- a/xen/include/xen/irq.h > +++ b/xen/include/xen/irq.h > @@ -91,6 +91,8 @@ typedef struct irq_desc { > spinlock_t lock; > struct arch_irq_desc arch; > cpumask_var_t affinity; > + void *data; /* IRQ specific data */ > + int virq; /* Used to store vlpi */ irq_desc is a common structure. We should not add new field without a strong argument. Currently, neither data and virq seems relevant: - the IRQ data is already contained in the irq_action. So why do you need to introduce a new field? - the virq field see https://patches.linaro.org/43012/ Regards, -- Julien Grall