From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932457AbdBHQsP (ORCPT <rfc822;w@1wt.eu>);
        Wed, 8 Feb 2017 11:48:15 -0500
Received: from protonic.xs4all.nl ([83.163.252.89]:2174 "EHLO
        protonic.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1754058AbdBHQsE (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 8 Feb 2017 11:48:04 -0500
Date: Wed, 8 Feb 2017 17:48:02 +0100
From: Robin van der Gracht <robin@protonic.nl>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH 2/3] auxdisplay: ht16k33: rework input device
 initialization
Message-ID: <20170208174802.26c370f6@erd979>
In-Reply-To: <20170131205438.7531-2-dmitry.torokhov@gmail.com>
References: <20170131205438.7531-1-dmitry.torokhov@gmail.com>
        <20170131205438.7531-2-dmitry.torokhov@gmail.com>
Organization: Protonic Holland
X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, 31 Jan 2017 12:54:37 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.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