Re: [RFC PATCH 09/19] xen/arm: Update irq descriptor for LPIs support

[Julien Grall <julien.grall@linaro.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