linux-kernel - Re: [PATCH] irqchip: gic: correct gic_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <53F6FE03.80409@arm.com>
Date:	Fri, 22 Aug 2014 09:23:31 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Feng Kan <fkan@....com>
CC:	"tglx@...utronix.de" <tglx@...utronix.de>,
	"jason@...edaemon.net" <jason@...edaemon.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dann.frazier@...onical.com" <dann.frazier@...onical.com>
Subject: Re: [PATCH] irqchip: gic: correct gic_set_type trigger acceptance
 criteria

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@....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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/