Re: [PATCH] irqchip: gic: correct gic_set_type trigger acceptance criteria

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Fen,

On 21/08/14 17:56, Feng Kan wrote:
> GIC is designed to support two types of trigger mechanism. Either active level
> high or edge rising.
> 
> static int gic_set_type(struct irq_data *d, unsigned int type)
> {
> 	...
>         if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>                 return -EINVAL;
> 
> However, this cause problem with requesting driver uses combo selections to tie
> down trigger mechanism. In below case gpio_keys_setup_key tries to use either
> rising or falling edge trigger, but accidently cause a false positive in the
> gic code.
> 
> static int gpio_keys_setup_key(struct platform_device *pdev,
> {
>                 irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
> 
> This patch fixes this problem by filter the selection type first.

I think this is the wrong thing to do. The semantic of the driver is to
receive an interrupt on both key-press and key-release. It needs both
events to function correctly.

While your patch enables the driver, it also only give it one of the two
expected events, and things will break. You should fix the gpio-keys
driver to only request one of the possible events (possibly detect when
it is failing to configure the interrupt).

Thanks,

	M.

> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  drivers/irqchip/irq-gic.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index c31eea4..bea167e 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -194,6 +194,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  	if (gicirq < 16)
>  		return -EINVAL;
>  
> +	type &= IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_EDGE_RISING;
>  	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>  		return -EINVAL;
>  
> 


-- 
Jazz is not dead. It just smells funny...