iio: st_sensors: harden interrupt handling

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.

We discovered that the ST hardware as far as can be observed
is designed for level interrupts: the DRDY line will be held
asserted as long as there are new values coming. The interrupt
handler should be re-entered until we're out of values to
handle from the sensor.

If interrupts were handled as occurring on the edges (usually
low-to-high) new values could appear and the line be held
asserted after that, and these values would be missed, the
interrupt handler would also lock up as new data was
available, but as no new edges occurs on the DRDY signal,
nothing happens: the edge detector only detects edges.

To counter this, do the following:

- Accept interrupt lines to be flagged as level interrupts
  using IRQF_TRIGGER_HIGH and IRQF_TRIGGER_LOW. If the line
  is marked like this (in the device tree node or ACPI
  table or similar) it will be utilized as a level IRQ.
  We mark the line with IRQF_ONESHOT and mask the IRQ
  while processing a sample, then the top half will be
  entered again if new values are available.

- If we are flagged as using edge interrupts with
  IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING: 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, then introduce
  a loop that polls the data ready registers repeatedly
  until no new samples are available, then exit the
  interrupt handler. This way we know no new values are
  available when the interrupt handler exits and
  new (edge) interrupts will be triggered when data arrives.
  Take some extra care to update the timestamp in the poll
  loop 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.

- If no interrupt type is indicated from the DT/ACPI,
  choose IRQF_TRIGGER_RISING as default, as this is necessary
  for legacy boards.

Tested successfully on the LIS331DL and L3G4200D by setting
sampling frequency to 400Hz/800Hz 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>
Tested-by: Crestez Dan Leonard <cdleonard@gmail.com>
Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jonathan Cameron <jic23@kernel.org>

drivers/iio/common/st_sensors/st_sensors_buffer.c

@@ -58,7 +58,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

drivers/iio/common/st_sensors/st_sensors_trigger.c

@@ -17,6 +17,50 @@
 #include <linux/iio/common/st_sensors.h>
 #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;
+	}
+	/*
+	 * the lower bits of .active_scan_mask[0] is directly mapped
+	 * to the channels on the sensor: either bit 0 for
+	 * one-dimensional sensors, or e.g. x,y,z for accelerometers,
+	 * gyroscopes or magnetometers. No sensor use more than 3
+	 * channels, so cut the other status bits here.
+	 */
+	status &= 0x07;
+
+	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
@@ -43,44 +87,43 @@ 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 (sdata->hw_irq_trigger &&
+	    st_sensors_new_samples_available(indio_dev, sdata)) {
+		iio_trigger_poll_chained(p);
+	} else {
+		dev_dbg(sdata->dev, "spurious IRQ\n");
+		return IRQ_NONE;
 	}
+
 	/*
-		 * the lower bits of .active_scan_mask[0] is directly mapped
-		 * to the channels on the sensor: either bit 0 for
-		 * one-dimensional sensors, or e.g. x,y,z for accelerometers,
-		 * gyroscopes or magnetometers. No sensor use more than 3
-		 * channels, so cut the other status bits here.
+	 * If we have proper level IRQs the handler will be re-entered if
+	 * the line is still active, so return here and come back in through
+	 * the top half if need be.
 	 */
-		status &= 0x07;
+	if (!sdata->edge_irq)
+		return IRQ_HANDLED;
 
 	/*
-		 * If this was not caused by any channels on this sensor,
-		 * return IRQ_NONE
+	 * If we are using egde IRQs, new samples arrived while processing
+	 * the IRQ and those may be missed unless we pick them here, so poll
+	 * again. If the sensor delivery frequency is very high, this thread
+	 * turns into a polled loop handler.
 	 */
-		if (!indio_dev->active_scan_mask)
-			return IRQ_NONE;
-		if (!(status & (u8)indio_dev->active_scan_mask[0]))
-			return IRQ_NONE;
+	while (sdata->hw_irq_trigger &&
+	       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(indio_dev);
+		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;
 }
 
@@ -107,13 +150,18 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 	 * If the IRQ is triggered on falling edge, we need to mark the
 	 * interrupt as active low, if the hardware supports this.
 	 */
-	if (irq_trig == IRQF_TRIGGER_FALLING) {
+	switch(irq_trig) {
+	case IRQF_TRIGGER_FALLING:
+	case IRQF_TRIGGER_LOW:
 		if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
 			dev_err(&indio_dev->dev,
-				"falling edge specified for IRQ but hardware "
-				"only support rising edge, will request "
-				"rising edge\n");
+				"falling/low specified for IRQ "
+				"but hardware only support rising/high: "
+				"will request rising/high\n");
+			if (irq_trig == IRQF_TRIGGER_FALLING)
 				irq_trig = IRQF_TRIGGER_RISING;
+			if (irq_trig == IRQF_TRIGGER_LOW)
+				irq_trig = IRQF_TRIGGER_HIGH;
 		} else {
 			/* Set up INT active low i.e. falling edge */
 			err = st_sensors_write_data_with_mask(indio_dev,
@@ -122,20 +170,39 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 			if (err < 0)
 				goto iio_trigger_free;
 			dev_info(&indio_dev->dev,
-				 "interrupts on the falling edge\n");
+				 "interrupts on the falling edge or "
+				 "active low level\n");
 		}
-	} else if (irq_trig == IRQF_TRIGGER_RISING) {
+		break;
+	case IRQF_TRIGGER_RISING:
 		dev_info(&indio_dev->dev,
 			 "interrupts on the rising edge\n");
-
-	} else {
+		break;
+	case IRQF_TRIGGER_HIGH:
+		dev_info(&indio_dev->dev,
+			 "interrupts active high level\n");
+		break;
+	default:
+		/* This is the most preferred mode, if possible */
 		dev_err(&indio_dev->dev,
-		"unsupported IRQ trigger specified (%lx), only "
-			"rising and falling edges supported, enforce "
+			"unsupported IRQ trigger specified (%lx), enforce "
 			"rising edge\n", irq_trig);
 		irq_trig = IRQF_TRIGGER_RISING;
 	}
 
+	/* Tell the interrupt handler that we're dealing with edges */
+	if (irq_trig == IRQF_TRIGGER_FALLING ||
+	    irq_trig == IRQF_TRIGGER_RISING)
+		sdata->edge_irq = true;
+	else
+		/*
+		 * If we're not using edges (i.e. level interrupts) we
+		 * just mask off the IRQ, handle one interrupt, then
+		 * if the line is still low, we return to the
+		 * interrupt handler top half again and start over.
+		 */
+		irq_trig |= IRQF_ONESHOT;
+
 	/*
 	 * If the interrupt pin is Open Drain, by definition this
 	 * means that the interrupt line may be shared with other
@@ -148,9 +215,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,

include/linux/iio/common/st_sensors.h

@@ -223,6 +223,7 @@ 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.
+ * @edge_irq: the IRQ triggers on edges and need special handling.
  * @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
  * @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
  */
@@ -250,6 +251,7 @@ struct st_sensor_data {
 	const struct st_sensor_transfer_function *tf;
 	struct st_sensor_transfer_buffer tb;
 
+	bool edge_irq;
 	bool hw_irq_trigger;
 	s64 hw_timestamp;
 };