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

[PATCH 1/2] 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 v4->v5:
- Move the setting of hw_irq_enabled to before the register
  write to enable interrupts so we avoid a race where this
  variable could be read in its previous state.
- NOTE: this patch is a regression fix and can be applied
  without 2/2.
- Leonard can you verify that this patch fixes your issue
  with HRTimer-triggered reads and provide your Tested-by?
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.
---
 drivers/iio/common/st_sensors/st_sensors_buffer.c  | 25 +++-----
 drivers/iio/common/st_sensors/st_sensors_core.c    |  3 +
 drivers/iio/common/st_sensors/st_sensors_trigger.c | 68 +++++++++++++++++++++-
 include/linux/iio/common/st_sensors.h              |  5 ++
 4 files changed, 80 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..928ee68fcc5f 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -424,6 +424,9 @@ int st_sensors_set_dataready_irq(struct iio_dev *indio_dev, bool enable)
 	else
 		drdy_mask = sdata->sensor_settings->drdy_irq.mask_int2;
 
+	/* Flag to the poll function that the hardware trigger is in use */
+	sdata->hw_irq_trigger = enable;
+
 	/* Enable/Disable the interrupt generator for data ready. */
 	err = st_sensors_write_data_with_mask(indio_dev,
 					sdata->sensor_settings->drdy_irq.addr,
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] 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, do two things:

- Remove IRQF_ONESHOT so that the interrupt line is not
  masked while running the thread part of the interrupt.
  This way we will never miss an interrupt.

- 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.

Usually the extra poll loop will handle the new samples,
but once in a blue moon, we get a new IRQ while exiting
the loop, before returning from the thread IRQ bottom half
with IRQ_HANDLED. On these rare occasions, the removal of
IRQF_ONESHOT means the interrupt will immediately fire
again.

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 v4->v5:
- Remove the IRQF_ONESHOT flag from the interrupt: this makes
  it possible to trigger the top half of the interrupt even
  though the bottom half is processing an earlier interrupt.
  This makes it possible to gracefully handle interrupts that
  come in during sensor reading.
- Add better descriptive comments.
- Leonard can you test this with your lagging sensor as well?
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_buffer.c  |  7 +-
 drivers/iio/common/st_sensors/st_sensors_trigger.c | 77 ++++++++++++++--------
 2 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index f1693dbebb8a..f6873919f188 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -59,7 +59,12 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 	s64 timestamp;
 
-	/* If we do timetamping here, do it before reading the values */
+	/*
+	 * If we do timetamping here, do it before reading the values, because
+	 * once we've read the values, new interrupts can occur (when using
+	 * the hardware trigger) and the hw_timestamp may get updated.
+	 * By storing it in a local variable first, we are safe.
+	 */
 	if (sdata->hw_irq_trigger)
 		timestamp = sdata->hw_timestamp;
 	else
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index c18e8221b6fa..f32145101bfe 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 or unknown
+ */
+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,28 @@ 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
+	 * status register, check if this IRQ came from us. Notice that
+	 * we will process also if st_sensors_new_samples_available()
+	 * returns negative: if we can't check status, then poll
+	 * unconditionally.
 	 */
-	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)) {
+		iio_trigger_poll_chained(p);
+	} else {
+		dev_dbg(sdata->dev, "spurious IRQ\n");
+		return IRQ_NONE;
+	}
 
-		/*
-		 * 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;
+	/* If new samples arrived while processing the IRQ, poll again. */
+	while (st_sensors_new_samples_available(indio_dev, sdata)) {
+		dev_dbg(sdata->dev, "more samples came in during polling\n");
+		sdata->hw_timestamp = iio_get_time_ns();
+		iio_trigger_poll_chained(p);
 	}
 
-out_poll:
-	/* It's our IRQ: proceed to handle the register polling */
-	iio_trigger_poll_chained(p);
 	return IRQ_HANDLED;
 }
 
@@ -136,9 +164,6 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 	    sdata->sensor_settings->drdy_irq.addr_stat_drdy)
 		irq_trig |= IRQF_SHARED;
 
-	/* 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,
-- 
2.4.11

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

[Crestez Dan Leonard]

On 05/09/2016 05:56 PM, Linus Walleij wrote:
> 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 v4->v5:
> - Move the setting of hw_irq_enabled to before the register
>   write to enable interrupts so we avoid a race where this
>   variable could be read in its previous state.
> - NOTE: this patch is a regression fix and can be applied
>   without 2/2.
> - Leonard can you verify that this patch fixes your issue
>   with HRTimer-triggered reads and provide your Tested-by?

Tested-by: Crestez Dan Leonard <leonard.crestez@intel.com>

> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +	/* 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();

This ignores pf->timestamp so passing iio_pollfunc_store_time no longer
has any effect, right? I think this is actually a good thing. When using
software triggers it is best to get the timestamp as close to the actual
read as possible. Relying on iio_pollfunc_store_time would just add the
noisy delay between hrtimer interrupts and process context to iio
timestamps.

But it would now make sense to pass NULL instead of
iio_pollfunc_store_time to all the iio_triggered_buffer_setup calls in
st_sensors, right?

It's also a bit strange that the buffer irq_handler is no longer called.
Maybe this needs validate_device in trigger_ops?

> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +	/*
> +	 * 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;

It's not clear that this belongs here. The check is not related to
sdata->sensor_settings->drdy_irq.addr_stat_drdy. It could be done before
checking the status register or even in the hardirq handler.

It seems to me that placing this check here would result in a bunch of
useless bus transactions.

-- 
Regards,
Leonard

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

[Crestez Dan Leonard]

On 05/09/2016 05:56 PM, Linus Walleij 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, do two things:
> 
> - Remove IRQF_ONESHOT so that the interrupt line is not
>   masked while running the thread part of the interrupt.
>   This way we will never miss an interrupt.
> 
> - 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.
> 
> Usually the extra poll loop will handle the new samples,
> but once in a blue moon, we get a new IRQ while exiting
> the loop, before returning from the thread IRQ bottom half
> with IRQ_HANDLED. On these rare occasions, the removal of
> IRQF_ONESHOT means the interrupt will immediately fire
> again.

I also tested this and it no longer stops printing samples. Your
explanation also looks good to me.

An infinite loop in the threaded interrupt handler looks odd. Are you
sure this is allowed? I do my tests in qemu with an usb adapter and if I
run with -smp 1 it seems to be impossible to stop the irq thread.

More specifically stopping the irq thread means something like:
	echo 0 > /sys/bus/iio/devices/iio:device0/buffer/enable
or
	echo 10 > /sys/bus/iio/devices/iio:device0/sampling_frequency
This seems to lockup waiting for the i2c adapter lock (but only with a
single CPU). Perhaps because the trigger thread only sleeps while
waiting for the i2c bus, which happens to be done while holding the i2c
adapter lock?

The simplest way to reproduce is to configure high frequency and run
generic_buffer with a -c 100 argument. After printing 100 samples it
will attempt to disable the buffer (by writing 0 to buffer/enable) but
hang inside i2c.

This issue might be specific to my test setup so not very interesting.

-- 
Regards,
Leonard

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

[Jonathan Cameron]

On 9 May 2016 19:36:23 BST, Crestez Dan Leonard <leonard.crestez@intel.com> wrote:
>On 05/09/2016 05:56 PM, Linus Walleij 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, do two things:
>> 
>> - Remove IRQF_ONESHOT so that the interrupt line is not
>>   masked while running the thread part of the interrupt.
>>   This way we will never miss an interrupt.
>> 
>> - 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.
>> 
>> Usually the extra poll loop will handle the new samples,
>> but once in a blue moon, we get a new IRQ while exiting
>> the loop, before returning from the thread IRQ bottom half
>> with IRQ_HANDLED. On these rare occasions, the removal of
>> IRQF_ONESHOT means the interrupt will immediately fire
>> again.
>
>I also tested this and it no longer stops printing samples. Your
>explanation also looks good to me.
>
>An infinite loop in the threaded interrupt handler looks odd. Are you
>sure this is allowed? I do my tests in qemu with an usb adapter and if
>I
>run with -smp 1 it seems to be impossible to stop the irq thread.

Definitely not allowed....

Long term we need a way to throttle or fault out after first few loops.

Right now this improves situation even if we just let it happen say 10 times to ride
our slight delays...

I ideally want to fire up my board and hammer this as well, plus read it very 
carefully to see if this really closes all potential races...


J
>
>More specifically stopping the irq thread means something like:
>	echo 0 > /sys/bus/iio/devices/iio:device0/buffer/enable
>or
>	echo 10 > /sys/bus/iio/devices/iio:device0/sampling_frequency
>This seems to lockup waiting for the i2c adapter lock (but only with a
>single CPU). Perhaps because the trigger thread only sleeps while
>waiting for the i2c bus, which happens to be done while holding the i2c
>adapter lock?
>
>The simplest way to reproduce is to configure high frequency and run
>generic_buffer with a -c 100 argument. After printing 100 samples it
>will attempt to disable the buffer (by writing 0 to buffer/enable) but
>hang inside i2c.
>
>This issue might be specific to my test setup so not very interesting.

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

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

[Linus Walleij]

On Mon, May 9, 2016 at 7:44 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> On 05/09/2016 05:56 PM, Linus Walleij wrote:

>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> +     /* 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();
>
> This ignores pf->timestamp so passing iio_pollfunc_store_time no longer
> has any effect, right? I think this is actually a good thing. When using
> software triggers it is best to get the timestamp as close to the actual
> read as possible. Relying on iio_pollfunc_store_time would just add the
> noisy delay between hrtimer interrupts and process context to iio
> timestamps.

Yeah that's right ... I didn't even think of that.

> But it would now make sense to pass NULL instead of
> iio_pollfunc_store_time to all the iio_triggered_buffer_setup calls in
> st_sensors, right?

Yep, I'm augmenting the patch to do that.

> It's also a bit strange that the buffer irq_handler is no longer called.
> Maybe this needs validate_device in trigger_ops?

Yeah that's true, it seems harmless but the mma8452 accelerometer
does this validation, it just compare if it's the same device, I'll add an
identical check for the st_sensors.

>> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
>> +     /*
>> +      * 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;
>
> It's not clear that this belongs here. The check is not related to
> sdata->sensor_settings->drdy_irq.addr_stat_drdy. It could be done before
> checking the status register or even in the hardirq handler.
>
> It seems to me that placing this check here would result in a bunch of
> useless bus transactions.

As you can see the read status is compared to the
active_scan_mask, this matches one-to-one to the enabled axis
and we check whether data is available on any of them.

We check sdata->sensor_settings->drdy_irq.addr_stat_drdy != NULL
to determine that the device has an DRDY status reqister.

If the sensor has this register (which is not a given) then we proceed
to check if it has indicated new values, i.e. if this sensor caused
the DRDY IRQ to happen.

If it doesn't have a status register, i.e.
sdata->sensor_settings->drdy_irq.addr_stat_drdy == NULL,
we cannot determine if we were the cause of this IRQ and we
proceed to poll the sensor anyways, assuming it was ours.
This makes it impossible to share the IRQ line or handle
spurious IRQs properly, but that is a hardware limitation.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, May 9, 2016 at 8:36 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> On 05/09/2016 05:56 PM, Linus Walleij wrote:

> I also tested this and it no longer stops printing samples. Your
> explanation also looks good to me.

OK we are moving in the right direction then!

> An infinite loop in the threaded interrupt handler looks odd. Are you
> sure this is allowed? I do my tests in qemu with an usb adapter and if I
> run with -smp 1 it seems to be impossible to stop the irq thread.

I will put in a limit to 10 as Jonathan recommended, let's see
if that fixes your problem! It's a sound limitation anyway.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, May 9, 2016 at 11:26 PM, Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:

>>An infinite loop in the threaded interrupt handler looks odd. Are you
>>sure this is allowed? I do my tests in qemu with an usb adapter and if
>>I
>>run with -smp 1 it seems to be impossible to stop the irq thread.
>
> Definitely not allowed....
>
> Long term we need a way to throttle or fault out after first few loops.
>
> Right now this improves situation even if we just let it happen say 10 times to ride
> our slight delays...

OK I'm putting in a hard limit to 10 loops.

> I ideally want to fire up my board and hammer this as well, plus read it very
> carefully to see if this really closes all potential races...

I'm doing my best, as far as I can tell this is how threaded
interrupts solve thing, normally.

As long as no hardirqs get masked, it should work. That
would only happen if a new IRQ happens while we're inside
the hard IRQ handler, so as long as this is fast enough (which
I guess is the assumption we make on all top halves) we
should be safe.

As the driver currently has a limitation and does not allow
to connect the sensor to an interrupt controller which itself
is threaded (such as a GPIO expander on an I2C bus) we
should be safe for the time being.

If we want to support the latter usecase at some point, we
may have problems: the I2C traffic to the expander itself
will maybe lead us to miss IRQs. But then it is really the
latency of the whole system causing things to break, not this
driver: the components used simply aren't fast enough.

I think in such cases the only safe way to read values is
to give up due to bad system architecture and to fall back to
using an HRTimer to poll the sensor.

Yours,
Linus Walleij