linux-kernel - Re: [PATCH 2/5] Input: omap4-keypad - scan keys in two phases and simplify with bitmask

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]
Message-ID: <X/vopuwCgxGxoVXs@atomide.com>
Date:   Mon, 11 Jan 2021 07:56:54 +0200
From:   Tony Lindgren <tony@...mide.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org,
        Arthur Demchenkov <spinal.by@...il.com>,
        Carl Philipp Klemm <philipp@...s.xyz>,
        Merlijn Wajer <merlijn@...zup.org>,
        Pavel Machek <pavel@....cz>, ruleh <ruleh@....de>,
        Sebastian Reichel <sre@...nel.org>
Subject: Re: [PATCH 2/5] Input: omap4-keypad - scan keys in two phases and
 simplify with bitmask

* Dmitry Torokhov <dmitry.torokhov@...il.com> [210111 04:48]:
> Technically speaking, userspace is free to accumulate the events until
> it receives EV_SYN/SYN_REPORT event and process the events in the event
> packet in order it sees fit. So to achieve what you want, I think we
> should issue 2 input_sync()s, one for the release block, and another is
> for press. I think we can also simplify the code if we pass into the new
> scan function exact set of keys that are being released or pressed.
> 
> How about the version below?

Yes that works nicely.

Thanks,

Tony


> 
> Input: omap4-keypad - scan keys in two phases and simplify with bitmask
> 
> From: Tony Lindgren <tony@...mide.com>
> 
> Because of errata i689 the keyboard can idle with state where no key
> up interrupts are seen until after the next key press.
> 
> This means we need to first check for any lost key up events before
> scanning for new down events.
> 
> For example, rapidly pressing shift-shift-j can sometimes produce a J
> instead of j. Let's fix the issue by scanning the keyboard in two
> phases. First we scan for any key up events that we may have missed,
> and then we scan for key down events.
> 
> Let's also simplify things with for_each_set_bit() as suggested by
> Dmitry Torokhov <dmitry.torokhov@...il.com>.
> 
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   73 ++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index ab761aa66b6d..6dcf27af856d 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -78,7 +78,7 @@ struct omap4_keypad {
>  	u32 irqreg_offset;
>  	unsigned int row_shift;
>  	bool no_autorepeat;
> -	unsigned char key_state[8];
> +	u64 keys;
>  	unsigned short *keymap;
>  };
>  
> @@ -107,6 +107,33 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
>  		     keypad_data->base + keypad_data->irqreg_offset + offset);
>  }
>  
> +static int omap4_keypad_report_keys(struct omap4_keypad *keypad_data,
> +				    u64 keys, bool down)
> +{
> +	struct input_dev *input_dev = keypad_data->input;
> +	unsigned int col, row, code;
> +	DECLARE_BITMAP(mask, 64);
> +	unsigned long bit;
> +	int events = 0;
> +
> +	bitmap_from_u64(mask, keys);
> +
> +	for_each_set_bit(bit, mask, keypad_data->rows * BITS_PER_BYTE) {
> +		row = bit / BITS_PER_BYTE;
> +		col = bit % BITS_PER_BYTE;
> +		code = MATRIX_SCAN_CODE(row, col, keypad_data->row_shift);
> +
> +		input_event(input_dev, EV_MSC, MSC_SCAN, code);
> +		input_report_key(input_dev, keypad_data->keymap[code], down);
> +
> +		events++;
> +	}
> +
> +	if (events)
> +		input_sync(input_dev);
> +
> +	return events;
> +}
>  
>  /* Interrupt handlers */
>  static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
> @@ -122,35 +149,25 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
>  static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
>  {
>  	struct omap4_keypad *keypad_data = dev_id;
> -	struct input_dev *input_dev = keypad_data->input;
> -	unsigned char key_state[ARRAY_SIZE(keypad_data->key_state)];
> -	unsigned int col, row, code, changed;
> -	u32 *new_state = (u32 *) key_state;
> -
> -	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> -	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
> -
> -	for (row = 0; row < keypad_data->rows; row++) {
> -		changed = key_state[row] ^ keypad_data->key_state[row];
> -		if (!changed)
> -			continue;
> -
> -		for (col = 0; col < keypad_data->cols; col++) {
> -			if (changed & (1 << col)) {
> -				code = MATRIX_SCAN_CODE(row, col,
> -						keypad_data->row_shift);
> -				input_event(input_dev, EV_MSC, MSC_SCAN, code);
> -				input_report_key(input_dev,
> -						 keypad_data->keymap[code],
> -						 key_state[row] & (1 << col));
> -			}
> -		}
> -	}
> +	u32 low, high;
> +	u64 keys, changed;
> +
> +	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> +	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
> +	keys = low | (u64)high << 32;
> +
> +	changed = keys ^ keypad_data->keys;
> +
> +	/*
> +	 * Report key up events separately and first. This matters in case we
> +	 * lost key-up interrupt and just now catching up.
> +	 */
> +	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
>  
> -	input_sync(input_dev);
> +	/* Report key down events */
> +	omap4_keypad_report_keys(keypad_data, changed & keys, true);
>  
> -	memcpy(keypad_data->key_state, key_state,
> -		sizeof(keypad_data->key_state));
> +	keypad_data->keys = keys;
>  
>  	/* clear pending interrupts */
>  	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,