[PATCH 1/2 v4] iio: st_sensors: switch to a threaded interrupt

[PATCH 1/2 v4] iio: st_sensors: switch to a threaded interrupt

[Linus Walleij]

commit 98ad8b41f58dff6b30713d7f09ae3834b8df7ded
("iio: st_sensors: verify interrupt event to status") caused
a regression when reading ST sensors from a HRTimer trigger
rather than the intrinsic interrupts: the HRTimer may
trigger faster than the sensor provides new values, and
as the check against new values available as a cause of
the interrupt trigger was done in the poll function,
this would bail out of the HRTimer interrupt with
IRQ_NONE.

So clearly we need to only check the new values available
from the proper interrupt handler and not from the poll
function, which should rather just read the raw values
from the registers, put them into the buffer and be happy.

To achieve this: switch the ST Sensors over to using a true
threaded interrupt handler.

In the interrupt thread, check if new values are available,
else yield to the (potential) next device on the same
interrupt line to check the registers. If the interrupt
was ours, proceed to poll the values.

Instead of relying on iio_trigger_generic_data_rdy_poll() as
a top half to wake up the thread that polls the sensor for
new data, have the thread call iio_trigger_poll_chained()
after determining that is is the proper source of the
interrupt. This is modelled on drivers/iio/accel/mma8452.c
which is already using a properly threaded interrupt handler.

In order to get the same precision in timestamps as
previously, where samples would be timestamped in the
poll function pf->timestamp when calling
iio_trigger_generic_data_rdy_poll() we introduce a
local timestamp in the sensor data, set it in the top half
(fastpath) of the interrupt handler and provide that to the
core when calling iio_push_to_buffers_with_timestamp().

Additionally: if the active scanmask is not set for the
sensor no IRQs should be enabled and we need to bail out
with IRQ_NONE. This can happen if spurious IRQs fire when
installing the threaded interrupt handler.

Tested with hard interrupt triggers on LIS331DL, then also
tested with hrtimers on the same sensor by creating a 75Hz
HRTimer and using it to poll the sensor.

Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Cc: Crestez Dan Leonard <cdleonard@gmail.com>
Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
Fixes: 97865fe41322 ("iio: st_sensors: verify interrupt event to status")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- v3 would not timestamp properly when using a HRTimer to read
  values from the sensors. This is now fixed.
- Add a flag to the sensor data indicating whether the hardware
  interrupt trigger is in use. If this is the case, we use that
  timestamp. If the hardware trigger is not in use, we just let
  the poll function read the current time before proceeding to
  grab the values from the sensor.
- Move interrupt top/bottom halves to st_sensors_trigger.c so the
  interrupt code is kept together and easier to understand in
  context.
ChangeLog v2->v3:
- v2 was a total disaster, as iio_trigger_generic_data_rdy_poll()
  would just call the old bottom half and return IRQ_HANDLED.
  handle the timestamp locally in the sensor data and restore
  the usage of the local interrupt thread.
- I think it works now.
ChangeLog v1->v2:
- v1 was missing timestamps since nothing ever stamped them.
  Restore the timestamps by simply using
  iio_trigger_generic_data_rdy_poll()
  as the top half of the threaded interrupt handler.

Crestez: please test this and check if it solves your
usecase too.
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c  | 25 +++-----
 drivers/iio/common/st_sensors/st_sensors_core.c    |  4 ++
 drivers/iio/common/st_sensors/st_sensors_trigger.c | 68 +++++++++++++++++++++-
 include/linux/iio/common/st_sensors.h              |  5 ++
 4 files changed, 81 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index c55898543a47..f1693dbebb8a 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -57,31 +57,20 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
+	s64 timestamp;
 
-	/* If we have a status register, check if this IRQ came from us */
-	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
-		u8 status;
-
-		len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
-			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
-			   &status);
-		if (len < 0)
-			dev_err(sdata->dev, "could not read channel status\n");
-
-		/*
-		 * If this was not caused by any channels on this sensor,
-		 * return IRQ_NONE
-		 */
-		if (!(status & (u8)indio_dev->active_scan_mask[0]))
-			return IRQ_NONE;
-	}
+	/* If we do timetamping here, do it before reading the values */
+	if (sdata->hw_irq_trigger)
+		timestamp = sdata->hw_timestamp;
+	else
+		timestamp = iio_get_time_ns();
 
 	len = st_sensors_get_buffer_element(indio_dev, sdata->buffer_data);
 	if (len < 0)
 		goto st_sensors_get_buffer_element_error;
 
 	iio_push_to_buffers_with_timestamp(indio_dev, sdata->buffer_data,
-		pf->timestamp);
+					   timestamp);
 
 st_sensors_get_buffer_element_error:
 	iio_trigger_notify_done(indio_dev->trig);
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index dffe00692169..eff423d64ecd 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -429,6 +429,10 @@ int st_sensors_set_dataready_irq(struct iio_dev *indio_dev, bool enable)
 					sdata->sensor_settings->drdy_irq.addr,
 					drdy_mask, (int)enable);
 
+	/* Flag to the poll function that the hardware trigger is in use */
+	if (!err)
+		sdata->hw_irq_trigger = enable;
+
 st_accel_set_dataready_irq_error:
 	return err;
 }
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index da72279fcf99..c18e8221b6fa 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -17,6 +17,65 @@
 #include <linux/iio/common/st_sensors.h>
 #include "st_sensors_core.h"
 
+/**
+ * st_sensors_irq_handler() - top half of the IRQ-based triggers
+ * @irq: irq number
+ * @p: private handler data
+ */
+irqreturn_t st_sensors_irq_handler(int irq, void *p)
+{
+	struct iio_trigger *trig = p;
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct st_sensor_data *sdata = iio_priv(indio_dev);
+
+	/* Get the time stamp as close in time as possible */
+	sdata->hw_timestamp = iio_get_time_ns();
+	return IRQ_WAKE_THREAD;
+}
+
+/**
+ * st_sensors_irq_thread() - bottom half of the IRQ-based triggers
+ * @irq: irq number
+ * @p: private handler data
+ */
+irqreturn_t st_sensors_irq_thread(int irq, void *p)
+{
+	struct iio_trigger *trig = p;
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct st_sensor_data *sdata = iio_priv(indio_dev);
+	int ret;
+
+	/*
+	 * If this trigger is backed by a hardware interrupt and we have a
+	 * status register, check if this IRQ came from us
+	 */
+	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
+		u8 status;
+
+		ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
+			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
+			   &status);
+		if (ret < 0) {
+			dev_err(sdata->dev, "could not read channel status\n");
+			goto out_poll;
+		}
+
+		/*
+		 * If this was not caused by any channels on this sensor,
+		 * return IRQ_NONE
+		 */
+		if (!indio_dev->active_scan_mask)
+			return IRQ_NONE;
+		if (!(status & (u8)indio_dev->active_scan_mask[0]))
+			return IRQ_NONE;
+	}
+
+out_poll:
+	/* It's our IRQ: proceed to handle the register polling */
+	iio_trigger_poll_chained(p);
+	return IRQ_HANDLED;
+}
+
 int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 				const struct iio_trigger_ops *trigger_ops)
 {
@@ -77,9 +136,12 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 	    sdata->sensor_settings->drdy_irq.addr_stat_drdy)
 		irq_trig |= IRQF_SHARED;
 
-	err = request_threaded_irq(irq,
-			iio_trigger_generic_data_rdy_poll,
-			NULL,
+	/* Let's create an interrupt thread masking the hard IRQ here */
+	irq_trig |= IRQF_ONESHOT;
+
+	err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
+			st_sensors_irq_handler,
+			st_sensors_irq_thread,
 			irq_trig,
 			sdata->trig->name,
 			sdata->trig);
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index d029ffac0d69..9bd1ec5c9c85 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -223,6 +223,8 @@ struct st_sensor_settings {
  * @get_irq_data_ready: Function to get the IRQ used for data ready signal.
  * @tf: Transfer function structure used by I/O operations.
  * @tb: Transfer buffers and mutex used by I/O operations.
+ * @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
+ * @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
  */
 struct st_sensor_data {
 	struct device *dev;
@@ -247,6 +249,9 @@ struct st_sensor_data {
 
 	const struct st_sensor_transfer_function *tf;
 	struct st_sensor_transfer_buffer tb;
+
+	bool hw_irq_trigger;
+	s64 hw_timestamp;
 };
 
 #ifdef CONFIG_IIO_BUFFER
-- 
2.4.11

[PATCH 2/2 v4] iio: st_sensors: read surplus samples in trigger

[Linus Walleij]

Leonard Crestez observed the following phenomenon: when using
hard interrupt triggers (the DRDY line coming out of an ST
sensor) sometimes a new value would arrive while reading the
previous value, due to latencies in the system.

As the interrupts from the ST sensors are a pulse, intended
to be read by an edge-triggered interrupt line (such as a
GPIO) one edge (transition from low-to-high or high-to-low)
will be missed while processing the current values.

If this happens, the state machine that triggers interrupts on
the DRDY line will lock waiting for the current value to be
read out and not fire any more interrupts. That means that
when we exit the interrupt handler, even though new values are
available from the sensor, no new interrupt will be triggered.

To counter this, introduce a loop that polls the data ready
registers repeatedly until no new samples are available,
then exits the interrupt handler. This way we know no new
values are available when the interrupt handler exits and new
interrupts will be triggered when data arrives.

Take some extra care to update the timestamp for the poll
function if this happens. The timestamp will not be 100%
perfect, but it will at least be closer to the actual events.

Tested successfully on the LIS331DL by setting sampling
frequency to 400Hz and stressing the system: extra reads in
the threaded interrupt handler occurs.

Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Cc: Crestez Dan Leonard <cdleonard@gmail.com>
Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Include this patch with the threaded interrupt fix patch.
  Adapte the same patch numbering system.

If this works I suggest it be applied to mainline as a fix on
top of the threading patch version 3, so we handle this annoying
lockup bug.
---
 drivers/iio/common/st_sensors/st_sensors_trigger.c | 65 ++++++++++++++++------
 1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index c18e8221b6fa..68d65f60fd25 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -18,6 +18,42 @@
 #include "st_sensors_core.h"
 
 /**
+ * st_sensors_new_samples_available() - check if more samples came in
+ * returns:
+ * 0 - no new samples available
+ * 1 - new samples available
+ * negative - error
+ */
+static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
+					    struct st_sensor_data *sdata)
+{
+	u8 status;
+	int ret;
+
+	/* How would I know if I can't check it? */
+	if (!sdata->sensor_settings->drdy_irq.addr_stat_drdy)
+		return -EINVAL;
+
+	/* No scan mask, no interrupt */
+	if (!indio_dev->active_scan_mask)
+		return 0;
+
+	ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
+			sdata->sensor_settings->drdy_irq.addr_stat_drdy,
+			&status);
+	if (ret < 0) {
+		dev_err(sdata->dev,
+			"error checking samples available\n");
+		return ret;
+	}
+
+	if (status & (u8)indio_dev->active_scan_mask[0])
+		return 1;
+
+	return 0;
+}
+
+/**
  * st_sensors_irq_handler() - top half of the IRQ-based triggers
  * @irq: irq number
  * @p: private handler data
@@ -43,36 +79,29 @@ irqreturn_t st_sensors_irq_thread(int irq, void *p)
 	struct iio_trigger *trig = p;
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
-	int ret;
 
 	/*
 	 * If this trigger is backed by a hardware interrupt and we have a
 	 * status register, check if this IRQ came from us
 	 */
-	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
-		u8 status;
-
-		ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
-			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
-			   &status);
-		if (ret < 0) {
-			dev_err(sdata->dev, "could not read channel status\n");
-			goto out_poll;
-		}
-
+	if (!st_sensors_new_samples_available(indio_dev, sdata))
 		/*
 		 * If this was not caused by any channels on this sensor,
 		 * return IRQ_NONE
 		 */
-		if (!indio_dev->active_scan_mask)
-			return IRQ_NONE;
-		if (!(status & (u8)indio_dev->active_scan_mask[0]))
-			return IRQ_NONE;
-	}
+		return IRQ_NONE;
 
-out_poll:
 	/* It's our IRQ: proceed to handle the register polling */
 	iio_trigger_poll_chained(p);
+
+	/* If new samples arrived while processing the IRQ, poll again */
+	while (st_sensors_new_samples_available(indio_dev, sdata) > 0) {
+		/* Timestamp and poll again */
+		sdata->hw_timestamp = iio_get_time_ns();
+		dev_dbg(sdata->dev, "more samples came in during polling\n");
+		iio_trigger_poll_chained(p);
+	}
+
 	return IRQ_HANDLED;
 }
 
-- 
2.4.11

Re: [PATCH 2/2 v4] iio: st_sensors: read surplus samples in trigger

[Jonathan Cameron]

On 7 May 2016 11:25:44 BST, Linus Walleij <linus.walleij@linaro.org> wrote:
>Leonard Crestez observed the following phenomenon: when using
>hard interrupt triggers (the DRDY line coming out of an ST
>sensor) sometimes a new value would arrive while reading the
>previous value, due to latencies in the system.
>
>As the interrupts from the ST sensors are a pulse, intended
>to be read by an edge-triggered interrupt line (such as a
>GPIO) one edge (transition from low-to-high or high-to-low)
>will be missed while processing the current values.
>
>If this happens, the state machine that triggers interrupts on
>the DRDY line will lock waiting for the current value to be
>read out and not fire any more interrupts. That means that
>when we exit the interrupt handler, even though new values are
>available from the sensor, no new interrupt will be triggered.
>
>To counter this, introduce a loop that polls the data ready
>registers repeatedly until no new samples are available,
>then exits the interrupt handler. This way we know no new
>values are available when the interrupt handler exits and new
>interrupts will be triggered when data arrives.
>
>Take some extra care to update the timestamp for the poll
>function if this happens. The timestamp will not be 100%
>perfect, but it will at least be closer to the actual events.
>
>Tested successfully on the LIS331DL by setting sampling
>frequency to 400Hz and stressing the system: extra reads in
>the threaded interrupt handler occurs.
>
>Cc: Giuseppe Barba <giuseppe.barba@st.com>
>Cc: Denis Ciocca <denis.ciocca@st.com>
>Cc: Crestez Dan Leonard <cdleonard@gmail.com>
>Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
>Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Shortens the race but doesn't prevent it that I can see..


In the old lis3l02dq driver in staging we ended up having to use gpio reads on
the interrupt line to clear this exact stall.

That can be done after we unmask the interrupt. Thus doesn't suffer the race.
Ugly though!

Also I think we broke the logic at somepoint  It calls trigger poll 
from threaded context.

Needs a rethink..
>---
>ChangeLog v3->v4:
>- Include this patch with the threaded interrupt fix patch.
>  Adapte the same patch numbering system.
>
>If this works I suggest it be applied to mainline as a fix on
>top of the threading patch version 3, so we handle this annoying
>lockup bug.
>---
>drivers/iio/common/st_sensors/st_sensors_trigger.c | 65
>++++++++++++++++------
> 1 file changed, 47 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c
>b/drivers/iio/common/st_sensors/st_sensors_trigger.c
>index c18e8221b6fa..68d65f60fd25 100644
>--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
>+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
>@@ -18,6 +18,42 @@
> #include "st_sensors_core.h"
> 
> /**
>+ * st_sensors_new_samples_available() - check if more samples came in
>+ * returns:
>+ * 0 - no new samples available
>+ * 1 - new samples available
>+ * negative - error
>+ */
>+static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
>+					    struct st_sensor_data *sdata)
>+{
>+	u8 status;
>+	int ret;
>+
>+	/* How would I know if I can't check it? */
>+	if (!sdata->sensor_settings->drdy_irq.addr_stat_drdy)
>+		return -EINVAL;
>+
>+	/* No scan mask, no interrupt */
>+	if (!indio_dev->active_scan_mask)
>+		return 0;
>+
>+	ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
>+			sdata->sensor_settings->drdy_irq.addr_stat_drdy,
>+			&status);
>+	if (ret < 0) {
>+		dev_err(sdata->dev,
>+			"error checking samples available\n");
>+		return ret;
>+	}
>+
>+	if (status & (u8)indio_dev->active_scan_mask[0])
>+		return 1;
>+
>+	return 0;
>+}
>+
>+/**
>  * st_sensors_irq_handler() - top half of the IRQ-based triggers
>  * @irq: irq number
>  * @p: private handler data
>@@ -43,36 +79,29 @@ irqreturn_t st_sensors_irq_thread(int irq, void *p)
> 	struct iio_trigger *trig = p;
> 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> 	struct st_sensor_data *sdata = iio_priv(indio_dev);
>-	int ret;
> 
> 	/*
> 	 * If this trigger is backed by a hardware interrupt and we have a
> 	 * status register, check if this IRQ came from us
> 	 */
>-	if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
>-		u8 status;
>-
>-		ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
>-			   sdata->sensor_settings->drdy_irq.addr_stat_drdy,
>-			   &status);
>-		if (ret < 0) {
>-			dev_err(sdata->dev, "could not read channel status\n");
>-			goto out_poll;
>-		}
>-
>+	if (!st_sensors_new_samples_available(indio_dev, sdata))
> 		/*
> 		 * If this was not caused by any channels on this sensor,
> 		 * return IRQ_NONE
> 		 */
>-		if (!indio_dev->active_scan_mask)
>-			return IRQ_NONE;
>-		if (!(status & (u8)indio_dev->active_scan_mask[0]))
>-			return IRQ_NONE;
>-	}
>+		return IRQ_NONE;
> 
>-out_poll:
> 	/* It's our IRQ: proceed to handle the register polling */
> 	iio_trigger_poll_chained(p);
>+
>+	/* If new samples arrived while processing the IRQ, poll again */
>+	while (st_sensors_new_samples_available(indio_dev, sdata) > 0) {
>+		/* Timestamp and poll again */
>+		sdata->hw_timestamp = iio_get_time_ns();
>+		dev_dbg(sdata->dev, "more samples came in during polling\n");
>+		iio_trigger_poll_chained(p);
>+	}
>+
> 	return IRQ_HANDLED;
> }
> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/2 v4] iio: st_sensors: read surplus samples in trigger

[Linus Walleij]

On Sat, May 7, 2016 at 1:15 PM, Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:

> Shortens the race but doesn't prevent it that I can see..

I thought about it tonight, and I need to add one more thing
to make the solution perfect: avoid registering the interrupt
with IRQF_ONESHOT. That way the top half will always be
executed if a new IRQ comes in. The top half will be really
fast and always ready to recieve new IRQs. As the IRQ is
always edge-triggered on these devices, we need not worry
about the IRQ handler locking up: IRQF_ONESHOT is
essentially for level IRQs that just keep the line low until
you served the IRQ.

Then we need the top half to tell the bottom half that a new
IRQ has arrived.

Right now I am thinking about letting the top half increase an
atomic counter any time a new IRQ comes in and that way
we can check from the thread if a new IRQ arrived before
exiting.

We still need to re-check the status from I2C reads again:
as the interrupt line may be shared, some other device
could have fired the IRQ, but as long as the *last* thing we
check before exiting the thread is the irq counter, we should
be pretty safe. It would have the same precision as polling
the GPIO line anyways.

Let me cook a patch like this and you can see what I mean!

Yours,
Linus Walleij

Re: [PATCH 2/2 v4] iio: st_sensors: read surplus samples in trigger

[Linus Walleij]

On Sun, May 8, 2016 at 10:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

> I thought about it tonight, and I need to add one more thing
> to make the solution perfect: avoid registering the interrupt
> with IRQF_ONESHOT.

After a bunch of tests it appears that this is actually all
that is needed.

The interrupt core will re-wake the thread if a second
IRQ came in to the hardirq (top half) during the processing
of the thread.

As we're checking status and dealing with shared
interrupts this makes sure we never miss events.

> Then we need the top half to tell the bottom half that a new
> IRQ has arrived.

So this is not needed.

Sending a v5.

Yours,
Linus Walleij