linux-kernel - Re: [PATCH 2/2] Input: omap-keypad: Cleanup - remove unnecessary IRQ enabling/disabling

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Fri, 19 Jul 2013 16:26:48 +0300
From:	Felipe Balbi <balbi@...com>
To:	Illia Smyrnov <illia.smyrnov@...ballogic.com>
CC:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	<linux-input@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/2] Input: omap-keypad: Cleanup - remove unnecessary IRQ
 enabling/disabling

Hi,

On Fri, Jul 19, 2013 at 04:03:47PM +0300, Illia Smyrnov wrote:
> From: Illia Smyrnov <illia.smyrnov@...com>
> 
> Remove unnecessary IRQ enabling/disabling for certain keyboard events.
> 
> Signed-off-by: Illia Smyrnov <illia.smyrnov@...com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index c727548..73813b6 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -121,10 +121,6 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  	unsigned int col, row, code, changed;
>  	u32 *new_state = (u32 *) key_state;
>  
> -	/* Disable interrupts */
> -	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
> -			 OMAP4_VAL_IRQDISABLE);
> -
>  	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
>  	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>  
> @@ -154,11 +150,6 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
>  			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
>  
> -	/* enable interrupts */
> -	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
> -		OMAP4_DEF_IRQENABLE_EVENTEN |
> -				OMAP4_DEF_IRQENABLE_LONGKEY);
> -

please don't remove this code. It'll be good to have this around when we
move the driver to threaded IRQs without IRQF_ONESHOT. In fact, it would
be very simple to implement such a change, wanna take it up ?

It should be doable in few patches:

1) switch over to request_threaded_irq()

	just blind move to a thread, without hardirq handler, so
	IRQF_ONESHOT is mandatory.

2) add hardirq handler

	read IRQSTATUS to check if our device has generated IRQs
	returning IRQ_WAKE_THREAD if true

3) move 'IRQ masking logic' to hardirq handler, before returning
IRQ_WAKE_THREAD

	this will let you remove IRQF_ONESHOT

4) finally remove IRQF_ONESHOT

	this makes sure that IRQs aren't kept disabled until we have
	time to iterate over the entire keypad matrix. Only the keypad
	IRQ will be masked.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)