Re: [PATCH 1/3] input: ps2-gpio: use ktime for IRQ timekeeping
From: Dmitry Torokhov
Date: Tue Feb 15 2022 - 01:14:36 EST
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@xxxxxxxxxxxxx>
> ---
> 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