linux-kernel - Re: [PATCH 2/3] auxdisplay: ht16k33: rework input device initialization

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: <20170208174802.26c370f6@erd979>
Date:   Wed, 8 Feb 2017 17:48:02 +0100
From:   Robin van der Gracht <robin@...tonic.nl>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Miguel Ojeda Sandonis <miguel.ojeda.sandonis@...il.com>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 2/3] auxdisplay: ht16k33: rework input device
 initialization

On Tue, 31 Jan 2017 12:54:37 -0800
Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:

...

> +
> +static irqreturn_t ht16k33_keypad_irq_thread(int irq, void *dev)
> +{
> +	struct ht16k33_keypad *keypad = dev;
> +
> +	do {
> +		wait_event_timeout(keypad->wait, keypad->stopped,
> +				    msecs_to_jiffies(keypad->debounce_ms));
> +		if (keypad->stopped)
> +			break;
> +	} while (ht16k33_keypad_scan(keypad));

I like how you worked this one out! Its way cleaner and simpler.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ht16k33_keypad_start(struct input_dev *dev)
> +{
> +	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
> +
> +	keypad->stopped = false;
> +	mb();
> +	enable_irq(keypad->client->irq);
> +
> +	return 0;
> +}
> +
> +static void ht16k33_keypad_stop(struct input_dev *dev)
> +{
> +	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
> +
> +	keypad->stopped = true;
> +	mb();
> +	wake_up(&keypad->wait);
> +	disable_irq(keypad->client->irq);
> +}

Nice!

> +
> +static int ht16k33_keypad_probe(struct i2c_client *client,
> +				struct ht16k33_keypad *keypad)
> +{

...

> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq,
> +					NULL, ht16k33_keypad_irq_thread,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					DRIVER_NAME, keypad);

I believe the interrupt trigger should be switched to IRQF_TRIGGER_HIGH
combined with IRQF_ONESHOT, for this new setup. This is causing a race
condition.

In the previous situation a 'key data' read was triggered by
ht16k33_keypad_start(). This cleared the IRQ and gave a (somewhat racy)
known state. Now, when the IRQ line is HIGH during probe keyscan
wont work. 

Also, when ht16k33_keypad_stop() is run while the irq thread is waiting
for debounce, the scan will be skipped and irq will never be cleared.
This also breaks keyscan.

Regards,
Robin