Re: [PATCH v2 16/16] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall <julien.grall@linaro.org>]

On 04/04/2014 08:59 AM, Jan Beulich wrote:
>>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>  unsigned int platform_get_irq(const struct dt_device_node *device,
>>                                int index);
>>  
>> +int request_irq_flags(unsigned int irq,
>> +                      void (*handler)(int, void *, struct cpu_user_regs *),
>> +                      unsigned int irqflags,
>> +                      const char * devname, void *dev_id);
>> +int setup_irq_flags(unsigned int irq, struct irqaction *new,
>> +                    unsigned int irqflags);
>> +
> 
> I think it is a bad choice to have separate ..._flags() versions of these,
> the normal functions should take flags at least when
> CONFIG_IRQ_HAS_MULTIPLE_ACTION. Perhaps, with the expectation
> of wanting to add further flags in the future, it would be reasonable to
> have the functions always have the flags parameters, and assert the
> value passed is zero on x86.

I chose this solution because I didn't know if extending the prototype
{request,setup}_irq would be acceptable.

With this patch series I've tried to remove ARM specific functions (e.g
{request,setup}_dt_irq) for IRQ management. So I would prefer to extend
the both function prototypes unconditionally to avoid ifdery in serial
drivers.

> 
>> @@ -27,6 +30,7 @@ struct irqaction {
>>  #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
>>  #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
>>  #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
>> +#define IRQ_SHARED        (1<<8)  /* IRQ is shared */
> 
> This is now given two meanings (input to above functions and status),
> which in the long run can become problematic. Please follow Linux by
> using two distinct flag sets.

Ok, I will rename it Ito IRQF_SHARED and introduce a new field "flags"
in irq_desc.

Regards,

-- 
Julien Grall