Changes since v1
================
- add patch to refactor struct ps2_gpio_data for clear separation between
RX and TX
- make all variables for IRQ timekeeping per-port and initialize them in
ps2_gpio_open()
This patch series implements the usage of ktime for IRQ timekeeping to
overcome:
(1) The resolution limitations of jiffies.
(2) Potential spurious IRQs generated by gpio controllers.
Besides that, based on the newly implemented timekeeping, it fixes a wrongly
suspected extra clock cycle for TX transfers and a race condition when
starting an immediate TX transfer based on data received from an RX transfer.
Danilo Krummrich (4):
input: ps2-gpio: refactor struct ps2_gpio_data
input: ps2-gpio: use ktime for IRQ timekeeping
input: ps2-gpio: remove tx timeout from ps2_gpio_irq_tx()
input: ps2-gpio: don't send rx data before the stop bit
drivers/input/serio/ps2-gpio.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------
1 file changed, 116 insertions(+), 64 deletions(-)
Refactor struct ps2_gpio_data in order to clearly separate RX and TX
state data.
This change intends to increase code readability and does not bring any
functional change.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/input/serio/ps2-gpio.c | 70 ++++++++++++++++++++--------------
1 file changed, 41 insertions(+), 29 deletions(-)
diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
index 8970b49ea09a..5f68505dd473 100644
--- a/drivers/input/serio/ps2-gpio.c
+++ b/drivers/input/serio/ps2-gpio.c
@@ -52,13 +52,17 @@ struct ps2_gpio_data {
struct gpio_desc *gpio_data;
bool write_enable;
int irq;
- unsigned char rx_cnt;
- unsigned char rx_byte;
- unsigned char tx_cnt;
- unsigned char tx_byte;
- struct completion tx_done;
- struct mutex tx_mutex;
- struct delayed_work tx_work;
+ struct ps2_gpio_data_rx {
+ unsigned char cnt;
+ unsigned char byte;
+ } rx;
+ struct ps2_gpio_data_tx {
+ unsigned char cnt;
+ unsigned char byte;
+ struct completion complete;
+ struct mutex mutex;
+ struct delayed_work work;
+ } tx;
};
static int ps2_gpio_open(struct serio *serio)
@@ -73,7 +77,7 @@ static void ps2_gpio_close(struct serio *serio)
{
struct ps2_gpio_data *drvdata = serio->port_data;
- flush_delayed_work(&drvdata->tx_work);
+ flush_delayed_work(&drvdata->tx.work);
disable_irq(drvdata->irq);
}
@@ -85,9 +89,9 @@ static int __ps2_gpio_write(struct serio *serio, unsigned char val)
gpiod_direction_output(drvdata->gpio_clk, 0);
drvdata->mode = PS2_MODE_TX;
- drvdata->tx_byte = val;
+ drvdata->tx.byte = val;
- schedule_delayed_work(&drvdata->tx_work, usecs_to_jiffies(200));
+ schedule_delayed_work(&drvdata->tx.work, usecs_to_jiffies(200));
return 0;
}
@@ -98,12 +102,12 @@ static int ps2_gpio_write(struct serio *serio, unsigned char val)
int ret = 0;
if (in_task()) {
- mutex_lock(&drvdata->tx_mutex);
+ mutex_lock(&drvdata->tx.mutex);
__ps2_gpio_write(serio, val);
- if (!wait_for_completion_timeout(&drvdata->tx_done,
+ if (!wait_for_completion_timeout(&drvdata->tx.complete,
msecs_to_jiffies(10000)))
ret = SERIO_TIMEOUT;
- mutex_unlock(&drvdata->tx_mutex);
+ mutex_unlock(&drvdata->tx.mutex);
} else {
__ps2_gpio_write(serio, val);
}
@@ -111,12 +115,20 @@ static int ps2_gpio_write(struct serio *serio, unsigned char val)
return ret;
}
+static inline struct ps2_gpio_data *
+to_ps2_gpio_data(struct delayed_work *dwork)
+{
+ struct ps2_gpio_data_tx *txd = container_of(dwork,
+ struct ps2_gpio_data_tx,
+ work);
+
+ return container_of(txd, struct ps2_gpio_data, tx);
+}
+
static void ps2_gpio_tx_work_fn(struct work_struct *work)
{
struct delayed_work *dwork = to_delayed_work(work);
- struct ps2_gpio_data *drvdata = container_of(dwork,
- struct ps2_gpio_data,
- tx_work);
+ struct ps2_gpio_data *drvdata = to_ps2_gpio_data(dwork);
enable_irq(drvdata->irq);
gpiod_direction_output(drvdata->gpio_data, 0);
@@ -130,8 +142,8 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
int rxflags = 0;
static unsigned long old_jiffies;
- byte = drvdata->rx_byte;
- cnt = drvdata->rx_cnt;
+ byte = drvdata->rx.byte;
+ cnt = drvdata->rx.cnt;
if (old_jiffies == 0)
old_jiffies = jiffies;
@@ -220,8 +232,8 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
old_jiffies = 0;
__ps2_gpio_write(drvdata->serio, PS2_CMD_RESEND);
end:
- drvdata->rx_cnt = cnt;
- drvdata->rx_byte = byte;
+ drvdata->rx.cnt = cnt;
+ drvdata->rx.byte = byte;
return IRQ_HANDLED;
}
@@ -231,8 +243,8 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data *drvdata)
int data;
static unsigned long old_jiffies;
- cnt = drvdata->tx_cnt;
- byte = drvdata->tx_byte;
+ cnt = drvdata->tx.cnt;
+ byte = drvdata->tx.byte;
if (old_jiffies == 0)
old_jiffies = jiffies;
@@ -284,7 +296,7 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data *drvdata)
}
drvdata->mode = PS2_MODE_RX;
- complete(&drvdata->tx_done);
+ complete(&drvdata->tx.complete);
cnt = 1;
old_jiffies = 0;
@@ -305,9 +317,9 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data *drvdata)
cnt = 1;
old_jiffies = 0;
gpiod_direction_input(drvdata->gpio_data);
- __ps2_gpio_write(drvdata->serio, drvdata->tx_byte);
+ __ps2_gpio_write(drvdata->serio, drvdata->tx.byte);
end:
- drvdata->tx_cnt = cnt;
+ drvdata->tx.cnt = cnt;
return IRQ_HANDLED;
}
@@ -403,11 +415,11 @@ static int ps2_gpio_probe(struct platform_device *pdev)
/* Tx count always starts at 1, as the start bit is sent implicitly by
* host-to-device communication initialization.
*/
- drvdata->tx_cnt = 1;
+ drvdata->tx.cnt = 1;
- INIT_DELAYED_WORK(&drvdata->tx_work, ps2_gpio_tx_work_fn);
- init_completion(&drvdata->tx_done);
- mutex_init(&drvdata->tx_mutex);
+ INIT_DELAYED_WORK(&drvdata->tx.work, ps2_gpio_tx_work_fn);
+ init_completion(&drvdata->tx.complete);
+ mutex_init(&drvdata->tx.mutex);
serio_register_port(serio);
platform_set_drvdata(pdev, drvdata);
--
2.35.1
Hi Danilo,
On Tue, Feb 15, 2022 at 05:02:05PM +0100, Danilo Krummrich wrote:
> +static inline struct ps2_gpio_data *
> +to_ps2_gpio_data(struct delayed_work *dwork)
> +{
> + struct ps2_gpio_data_tx *txd = container_of(dwork,
> + struct ps2_gpio_data_tx,
> + work);
> +
> + return container_of(txd, struct ps2_gpio_data, tx);
> +}
> +
> static void ps2_gpio_tx_work_fn(struct work_struct *work)
> {
> struct delayed_work *dwork = to_delayed_work(work);
> - struct ps2_gpio_data *drvdata = container_of(dwork,
> - struct ps2_gpio_data,
> - tx_work);
This can simply be written as:
struct ps2_gpio_data *drvdata = container_of(dwork,
struct ps2_gpio_data,
tx.work);
No need to resubmit unless you disagree - I can change it on my side.
Thanks.
--
Dmitry
On Tue, Feb 15, 2022 at 05:02:04PM +0100, Danilo Krummrich wrote:
> Changes since v1
> ================
> - add patch to refactor struct ps2_gpio_data for clear separation between
> RX and TX
> - make all variables for IRQ timekeeping per-port and initialize them in
> ps2_gpio_open()
>
> This patch series implements the usage of ktime for IRQ timekeeping to
> overcome:
>
> (1) The resolution limitations of jiffies.
> (2) Potential spurious IRQs generated by gpio controllers.
>
> Besides that, based on the newly implemented timekeeping, it fixes a wrongly
> suspected extra clock cycle for TX transfers and a race condition when
> starting an immediate TX transfer based on data received from an RX transfer.
>
> Danilo Krummrich (4):
> input: ps2-gpio: refactor struct ps2_gpio_data
> input: ps2-gpio: use ktime for IRQ timekeeping
> input: ps2-gpio: remove tx timeout from ps2_gpio_irq_tx()
> input: ps2-gpio: don't send rx data before the stop bit
>
> drivers/input/serio/ps2-gpio.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------
> 1 file changed, 116 insertions(+), 64 deletions(-)
Applied the lot, thank you.
--
Dmitry
On Tue, Feb 15, 2022 at 11:23:17PM +0100, Danilo Krummrich wrote:
> Hi Dmitry,
>
> On Tue, Feb 15, 2022 at 01:54:46PM -0800, Dmitry Torokhov wrote:
> > Hi Danilo,
> >
> > On Tue, Feb 15, 2022 at 05:02:05PM +0100, Danilo Krummrich wrote:
> >
> > > +static inline struct ps2_gpio_data *
> > > +to_ps2_gpio_data(struct delayed_work *dwork)
> > > +{
> > > + struct ps2_gpio_data_tx *txd = container_of(dwork,
> > > + struct ps2_gpio_data_tx,
> > > + work);
> > > +
> > > + return container_of(txd, struct ps2_gpio_data, tx);
> > > +}
> > > +
> > > static void ps2_gpio_tx_work_fn(struct work_struct *work)
> > > {
> > > struct delayed_work *dwork = to_delayed_work(work);
> > > - struct ps2_gpio_data *drvdata = container_of(dwork,
> > > - struct ps2_gpio_data,
> > > - tx_work);
> >
> > This can simply be written as:
> >
> > struct ps2_gpio_data *drvdata = container_of(dwork,
> > struct ps2_gpio_data,
> > tx.work);
> >
> > No need to resubmit unless you disagree - I can change it on my side.
> Thanks, please do so.
>
> The tx and rx members of struct ps2_gpio_data can then be anonymous structs.
> Do you mind changing that too? Or should I resubmit?
I will.
Thanks.
--
Dmitry
Hi Dmitry,
On Tue, Feb 15, 2022 at 01:54:46PM -0800, Dmitry Torokhov wrote:
> Hi Danilo,
>
> On Tue, Feb 15, 2022 at 05:02:05PM +0100, Danilo Krummrich wrote:
>
> > +static inline struct ps2_gpio_data *
> > +to_ps2_gpio_data(struct delayed_work *dwork)
> > +{
> > + struct ps2_gpio_data_tx *txd = container_of(dwork,
> > + struct ps2_gpio_data_tx,
> > + work);
> > +
> > + return container_of(txd, struct ps2_gpio_data, tx);
> > +}
> > +
> > static void ps2_gpio_tx_work_fn(struct work_struct *work)
> > {
> > struct delayed_work *dwork = to_delayed_work(work);
> > - struct ps2_gpio_data *drvdata = container_of(dwork,
> > - struct ps2_gpio_data,
> > - tx_work);
>
> This can simply be written as:
>
> struct ps2_gpio_data *drvdata = container_of(dwork,
> struct ps2_gpio_data,
> tx.work);
>
> No need to resubmit unless you disagree - I can change it on my side.
Thanks, please do so.
The tx and rx members of struct ps2_gpio_data can then be anonymous structs.
Do you mind changing that too? Or should I resubmit?
>
> Thanks.
>
> --
> Dmitry
- Danilo
Sending the data before processing the stop bit from the device already
saves the data of the current xfer in case the stop bit is missed.
However, when TX xfers are enabled this introduces a race condition when
a peripheral driver using the bus immediately requests a TX xfer from IRQ
context.
Therefore the data must be send after receiving the stop bit, although
it is possible the data is lost when missing the stop bit.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/input/serio/ps2-gpio.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
index f47a967f7521..17091b137744 100644
--- a/drivers/input/serio/ps2-gpio.c
+++ b/drivers/input/serio/ps2-gpio.c
@@ -231,6 +231,13 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
if (!drvdata->write_enable)
goto err;
}
+ break;
+ case PS2_STOP_BIT:
+ /* stop bit should be high */
+ if (unlikely(!data)) {
+ dev_err(drvdata->dev, "RX: stop bit should be high\n");
+ goto err;
+ }
/* Do not send spurious ACK's and NACK's when write fn is
* not provided.
@@ -242,21 +249,9 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data *drvdata)
break;
}
- /* Let's send the data without waiting for the stop bit to be
- * sent. It may happen that we miss the stop bit. When this
- * happens we have no way to recover from this, certainly
- * missing the parity bit would be recognized when processing
- * the stop bit. When missing both, data is lost.
- */
serio_interrupt(drvdata->serio, byte, rxflags);
dev_dbg(drvdata->dev, "RX: sending byte 0x%x\n", byte);
- break;
- case PS2_STOP_BIT:
- /* stop bit should be high */
- if (unlikely(!data)) {
- dev_err(drvdata->dev, "RX: stop bit should be high\n");
- goto err;
- }
+
cnt = byte = 0;
goto end; /* success */
--
2.35.1