Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 14 Feb 2022 22:14:24 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Danilo Krummrich <danilokrummrich@dk-develop.de>
Cc:     linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        linus.walleij@linaro.org
Subject: Re: [PATCH 1/3] input: ps2-gpio: use ktime for IRQ timekeeping
Message-ID: <YgtEwMQMrp3uQinK@google.com>
References: <20220211212258.80345-1-danilokrummrich@dk-develop.de>
 <20220211212258.80345-2-danilokrummrich@dk-develop.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220211212258.80345-2-danilokrummrich@dk-develop.de>
Precedence: bulk

Hi Danilo,

On Fri, Feb 11, 2022 at 10:22:56PM +0100, Danilo Krummrich wrote:
> Using jiffies for the IRQ timekeeping is not sufficient for two reasons:
> 
> (1) Usually jiffies have a resolution of 1ms to 10ms. The IRQ intervals
>     based on the clock frequency of PS2 protocol specification (10kHz -
>     16.7kHz) are between ~60us and 100us only. Therefore only those IRQ
>     intervals can be detected which are either at the end of a transfer
>     or are overly delayed. While this is sufficient in most cases, since
>     we have quite a lot of ways to detect faulty transfers, it can
>     produce false positives in rare cases: When the jiffies value
>     changes right between two interrupt that are in time, we wrongly
>     assume that we missed one or more clock cycles.
> 
> (2) Some gpio controllers (e.g. the one in the bcm283x chips) may generate
>     spurious IRQs when processing interrupts in the frequency given by PS2
>     devices.
> 
> Both issues can be fixed by using ktime resolution for IRQ timekeeping.
> 
> However, it is still possible to miss clock cycles without detecting
> them. When the PS2 device generates the falling edge of the clock signal
> we have between ~30us and 50us to sample the data line, because after
> this time we reach the next rising edge at which the device changes the
> data signal already. But, the only thing we can detect is whether the
> IRQ interval is within the given period. Therefore it is possible to
> have an IRQ latency greater than ~30us to 50us, sample the wrong bit on
> the data line and still be on time with the next IRQ. However, this can
> only happen when within a given transfer the IRQ latency increases
> slowly.
> 
> ___            ______            ______            ______            ___
>    \          /      \          /      \          /      \          /
>     \        /        \        /        \        /        \        /
>      \______/          \______/          \______/          \______/
> 
>     |-----------------|                 |--------|
>          60us/100us                      30us/50us
> 
> Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
> ---
>  drivers/input/serio/ps2-gpio.c | 81 ++++++++++++++++++++++++++++------
>  1 file changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
> index 8970b49ea09a..7fef4176bdd1 100644
> --- a/drivers/input/serio/ps2-gpio.c
> +++ b/drivers/input/serio/ps2-gpio.c
> @@ -19,6 +19,7 @@
>  #include <linux/of.h>
>  #include <linux/jiffies.h>
>  #include <linux/delay.h>
> +#include <linux/timekeeping.h>
>  
>  #define DRIVER_NAME		"ps2-gpio"
>  
> @@ -44,6 +45,29 @@
>  
>  #define PS2_CMD_RESEND		0xfe
>  
> +/* The PS2 protocol specifies a clock frequency between 10kHz and 16.7kHz,
> + * therefore the maximal interrupt interval should be 100us and the minimum
> + * interrupt interval should be ~60us. Let's allow +/- 20us for frequency
> + * deviations and interrupt latency.
> + *
> + * The data line must be samples after ~30us to 50us after the falling edge,
> + * since the device updates the data line at the rising edge.
> + *
> + * ___            ______            ______            ______            ___
> + *    \          /      \          /      \          /      \          /
> + *     \        /        \        /        \        /        \        /
> + *      \______/          \______/          \______/          \______/
> + *
> + *     |-----------------|                 |--------|
> + *          60us/100us                      30us/50us
> + */
> +#define PS2_CLK_FREQ_MIN_HZ		10000
> +#define PS2_CLK_FREQ_MAX_HZ		16700
> +#define PS2_CLK_MIN_INTERVAL_US		((1000 * 1000) / PS2_CLK_FREQ_MAX_HZ)
> +#define PS2_CLK_MAX_INTERVAL_US		((1000 * 1000) / PS2_CLK_FREQ_MIN_HZ)
> +#define PS2_IRQ_MIN_INTERVAL_US		(PS2_CLK_MIN_INTERVAL_US - 20)
> +#define PS2_IRQ_MAX_INTERVAL_US		(PS2_CLK_MAX_INTERVAL_US + 20)
> +
>  struct ps2_gpio_data {
>  	struct device *dev;
>  	struct serio *serio;
> @@ -59,6 +83,8 @@ struct ps2_gpio_data {
>  	struct completion tx_done;
>  	struct mutex tx_mutex;
>  	struct delayed_work tx_work;
> +	ktime_t tx_start;
> +	ktime_t tx_end;
>  };
>  
>  static int ps2_gpio_open(struct serio *serio)
> @@ -118,6 +144,7 @@ static void ps2_gpio_tx_work_fn(struct work_struct *work)
>  						    struct ps2_gpio_data,
>  						    tx_work);
>  
> +	drvdata->tx_start = ktime_get();
>  	enable_irq(drvdata->irq);
>  	gpiod_direction_output(drvdata->gpio_data, 0);
>  	gpiod_direction_input(drvdata->gpio_clk);
> @@ -128,20 +155,33 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
>  	unsigned char byte, cnt;
>  	int data;
>  	int rxflags = 0;
> -	static unsigned long old_jiffies;
> +	static ktime_t t_last, t_now;
> +	s64 us_delta;
>  
>  	byte = drvdata->rx_byte;
>  	cnt = drvdata->rx_cnt;
>  
> -	if (old_jiffies == 0)
> -		old_jiffies = jiffies;
> +	t_now = ktime_get();
> +	if (t_last == 0)

Instead of checking this every time, do you think we could seed the
value in ps2_gpio_open() (and also make it per-port, not static)?

> +		t_last = t_now;
>  
> -	if ((jiffies - old_jiffies) > usecs_to_jiffies(100)) {
> +	/* We need to consider spurious interrupts happening right after a TX xfer
> +	 * finished.
> +	 */
> +	if (unlikely(ktime_us_delta(t_now, drvdata->tx_end) <
> +		     PS2_IRQ_MIN_INTERVAL_US))
> +		goto end;
> +
> +	us_delta = ktime_us_delta(t_now, t_last);
> +	if (us_delta > PS2_IRQ_MAX_INTERVAL_US && cnt) {
>  		dev_err(drvdata->dev,
>  			"RX: timeout, probably we missed an interrupt\n");
>  		goto err;
> +	} else if (us_delta < PS2_IRQ_MIN_INTERVAL_US && t_now != t_last) {
> +		/* Ignore spurious IRQs. */
> +		goto end;
>  	}
> -	old_jiffies = jiffies;
> +	t_last = t_now;
>  
>  	data = gpiod_get_value(drvdata->gpio_data);
>  	if (unlikely(data < 0)) {
> @@ -205,7 +245,7 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
>  			goto err;
>  		}
>  		cnt = byte = 0;
> -		old_jiffies = 0;
> +
>  		goto end; /* success */
>  	default:
>  		dev_err(drvdata->dev, "RX: got out of sync with the device\n");
> @@ -217,7 +257,6 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
>  
>  err:
>  	cnt = byte = 0;
> -	old_jiffies = 0;
>  	__ps2_gpio_write(drvdata->serio, PS2_CMD_RESEND);
>  end:
>  	drvdata->rx_cnt = cnt;
> @@ -229,20 +268,34 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data *drvdata)
>  {
>  	unsigned char byte, cnt;
>  	int data;
> -	static unsigned long old_jiffies;
> +	static ktime_t t_last, t_now;
> +	s64 us_delta;
>  
>  	cnt = drvdata->tx_cnt;
>  	byte = drvdata->tx_byte;
>  
> -	if (old_jiffies == 0)
> -		old_jiffies = jiffies;
> +	t_now = ktime_get();
> +	if (t_last == 0)
> +		t_last = t_now;
> +
> +	/* There might be pending IRQs since we disabled IRQs in __ps2_gpio_write().
> +	 * We can expect at least one clock period until the device generates the
> +	 * first falling edge after releasing the clock line.
> +	 */
> +	if (unlikely(ktime_us_delta(t_now, drvdata->tx_start) <
> +		     PS2_CLK_MIN_INTERVAL_US))
> +		goto end;
>  
> -	if ((jiffies - old_jiffies) > usecs_to_jiffies(100)) {
> +	us_delta = ktime_us_delta(t_now, t_last);
> +	if (us_delta > PS2_IRQ_MAX_INTERVAL_US && cnt > 1) {
>  		dev_err(drvdata->dev,
>  			"TX: timeout, probably we missed an interrupt\n");
>  		goto err;
> +	} else if (us_delta < PS2_IRQ_MIN_INTERVAL_US && t_now != t_last) {
> +		/* Ignore spurious IRQs. */
> +		goto end;
>  	}
> -	old_jiffies = jiffies;
> +	t_last = t_now;
>  
>  	switch (cnt) {
>  	case PS2_START_BIT:
> @@ -283,11 +336,11 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data *drvdata)
>  			goto err;
>  		}
>  
> +		drvdata->tx_end = ktime_get();
>  		drvdata->mode = PS2_MODE_RX;
>  		complete(&drvdata->tx_done);
>  
>  		cnt = 1;
> -		old_jiffies = 0;
>  		goto end; /* success */
>  	default:
>  		/* Probably we missed the stop bit. Therefore we release data
> @@ -303,7 +356,6 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data *drvdata)
>  
>  err:
>  	cnt = 1;
> -	old_jiffies = 0;
>  	gpiod_direction_input(drvdata->gpio_data);
>  	__ps2_gpio_write(drvdata->serio, drvdata->tx_byte);
>  end:
> @@ -399,6 +451,7 @@ static int ps2_gpio_probe(struct platform_device *pdev)
>  	drvdata->serio = serio;
>  	drvdata->dev = dev;
>  	drvdata->mode = PS2_MODE_RX;
> +	drvdata->tx_end = 0;
>  
>  	/* Tx count always starts at 1, as the start bit is sent implicitly by
>  	 * host-to-device communication initialization.
> -- 
> 2.34.1
> 

Thanks.

-- 
Dmitry