[PATCH v3 0/6] Adis IRQ fixes and minor improvements

[PATCH v3 0/6] Adis IRQ fixes and minor improvements

[Nuno Sa]

The primary goal of this series was to fix the return value on some
trigger handlers as spotted in [1]. While doing it, I found some minor
improvements that I think are simple enough to include in this series.

As for the first 2 patches, I opted to not do any functional change so
I'm keeping the 'if (!adis->buffer)' check. However, 'adis-buffer' is
allocated in 'update_scan_mode' hook which means we should be sure that
the buffer is allocated and the check is really not needed. I did not
went into the details but is there any way for the trigger handler to be
called before the 'update_scan_mode' hook? If not, I'm happy in sending
a v2 where we just remove the 'if'...


Changes in v2:
 * Remove the 'if' check for the allocated buffer;
 * Make sure the adis 'state_lock' is unlocked on error paths;
 * Fixed the commit message on the first 3 patches.
 * Dropped ("iio: adis16475: re-set max spi transfer") and added 3 new
patches (last 3 in the series ) to fix a potential race with the spi core
as discussed in [2].

Changes in v3:
 * Improved commit description on ("iio: adis16475: do not return ints in
irq handlers");
 * Reordered patch 2 and 3 so that patch 3 is easier to apply;
 * Re-arranged the error handling on 'spi_sync()' error path.

[1]: https://marc.info/?l=linux-iio&m=161815197426882&w=2
[2]: https://marc.info/?l=linux-iio&m=161884696722142&w=2

Nuno Sa (6):
  iio: adis16475: do not return ints in irq handlers
  iio: adis_buffer: update device page after changing it
  iio: adis_buffer: don't push data to buffers on failure
  iio: adis: add burst_max_speed_hz variable
  iio: adis16475: do not directly change spi 'max_speed_hz'
  iio: adis16400: do not directly change spi 'max_speed_hz'

 drivers/iio/imu/adis16400.c   | 15 ++-------------
 drivers/iio/imu/adis16475.c   |  9 +++------
 drivers/iio/imu/adis_buffer.c | 16 ++++++++++------
 include/linux/iio/imu/adis.h  |  2 ++
 4 files changed, 17 insertions(+), 25 deletions(-)

-- 
2.31.1

[PATCH v3 1/6] iio: adis16475: do not return ints in irq handlers

[Nuno Sa]

On an IRQ handler we should not return normal error codes as 'irqreturn_t'
is expected.

This is done by jumping to the 'check_burst32' label where we return
'IRQ_HANDLED'. Note that it is fine to do the burst32 check in this
error path. If we have proper settings to apply burst32, we might just
do the setup now so that the next sample already uses it.

Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 1de62fc79e0f..51b76444db0b 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -1068,7 +1068,7 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
 
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret)
-		return ret;
+		goto check_burst32;
 
 	adis->spi->max_speed_hz = cached_spi_speed_hz;
 	buffer = adis->buffer;
-- 
2.31.1

[PATCH v3 2/6] iio: adis_buffer: update device page after changing it

[Nuno Sa]

It makes more sense to update the device page as soon as we we
successfully changed it. Moreover, a follow up patch will handle
'spi_sync' error path which would leave 'current_page' in a inconsistent
state.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 0ae551a748eb..4fc0e0ca7561 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 				mutex_unlock(&adis->state_lock);
 				goto irq_done;
 			}
+
+			adis->current_page = 0;
 		}
 	}
 
@@ -148,10 +150,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
 
 
-	if (adis->data->has_paging) {
-		adis->current_page = 0;
+	if (adis->data->has_paging)
 		mutex_unlock(&adis->state_lock);
-	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
 		pf->timestamp);
-- 
2.31.1

[PATCH v3 3/6] iio: adis_buffer: don't push data to buffers on failure

[Nuno Sa]

There's no point in pushing data to IIO buffers in case 'spi_sync()'
fails.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 4fc0e0ca7561..f6dbfbd17d41 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -146,12 +146,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	}
 
 	ret = spi_sync(adis->spi, &adis->msg);
-	if (ret)
-		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
-
-
 	if (adis->data->has_paging)
 		mutex_unlock(&adis->state_lock);
+	if (ret) {
+		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
+		goto irq_done;
+	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
 		pf->timestamp);
-- 
2.31.1

[PATCH v3 4/6] iio: adis: add burst_max_speed_hz variable

[Nuno Sa]

Typically, in burst mode, the device cannot operate at it's full spi
speed. Hence, the spi transfers for burst mode have to take this into
account. With this change we avoid a potential race with the spi core as
drivers were 'hacking' the device 'max_speed_hz' directly in the
trigger handler.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 4 ++++
 include/linux/iio/imu/adis.h  | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index f6dbfbd17d41..351c303c8a8c 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -51,9 +51,13 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
 	adis->xfer[0].tx_buf = tx;
 	adis->xfer[0].bits_per_word = 8;
 	adis->xfer[0].len = 2;
+	if (adis->data->burst_max_speed_hz)
+		adis->xfer[0].speed_hz = adis->data->burst_max_speed_hz;
 	adis->xfer[1].rx_buf = adis->buffer;
 	adis->xfer[1].bits_per_word = 8;
 	adis->xfer[1].len = burst_length;
+	if (adis->data->burst_max_speed_hz)
+		adis->xfer[1].speed_hz = adis->data->burst_max_speed_hz;
 
 	spi_message_init(&adis->msg);
 	spi_message_add_tail(&adis->xfer[0], &adis->msg);
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index f9b728d490b1..cf49997d5903 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -55,6 +55,7 @@ struct adis_timeout {
  *			this should be the minimum size supported by the device.
  * @burst_max_len:	Holds the maximum burst size when the device supports
  *			more than one burst mode with different sizes
+ * @burst_max_speed_hz:	Maximum spi speed that can be used in burst mode
  */
 struct adis_data {
 	unsigned int read_delay;
@@ -83,6 +84,7 @@ struct adis_data {
 	unsigned int burst_reg_cmd;
 	unsigned int burst_len;
 	unsigned int burst_max_len;
+	unsigned int burst_max_speed_hz;
 };
 
 /**
-- 
2.31.1

[PATCH v3 5/6] iio: adis16475: do not directly change spi 'max_speed_hz'

[Nuno Sa]

With commit 3ba10e37371d ("iio: adis: add burst_max_speed_hz variable"), we
just need to define 'burst_max_speed_hz' and the adis core will take
care of setting up the spi transfers for burst mode. Hence, we fix
a potential race with the spi core where we could be left with an
invalid 'max_speed_hz'.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 51b76444db0b..5654c0c15426 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -645,7 +645,8 @@ static int adis16475_enable_irq(struct adis *adis, bool enable)
 	.timeouts = (_timeouts),					\
 	.burst_reg_cmd = ADIS16475_REG_GLOB_CMD,			\
 	.burst_len = ADIS16475_BURST_MAX_DATA,				\
-	.burst_max_len = ADIS16475_BURST32_MAX_DATA			\
+	.burst_max_len = ADIS16475_BURST32_MAX_DATA,			\
+	.burst_max_speed_hz = ADIS16475_BURST_MAX_SPEED			\
 }
 
 static const struct adis16475_sync adis16475_sync_mode[] = {
@@ -1062,15 +1063,11 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
 	bool valid;
 	/* offset until the first element after gyro and accel */
 	const u8 offset = st->burst32 ? 13 : 7;
-	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
-
-	adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
 
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret)
 		goto check_burst32;
 
-	adis->spi->max_speed_hz = cached_spi_speed_hz;
 	buffer = adis->buffer;
 
 	crc = be16_to_cpu(buffer[offset + 2]);
-- 
2.31.1

[PATCH v3 6/6] iio: adis16400: do not directly change spi 'max_speed_hz'

[Nuno Sa]

With commit 3ba10e37371d ("iio: adis: add burst_max_speed_hz variable"), we
just need to define 'burst_max_speed_hz' and the adis core will take
care of setting up the spi transfers for burst mode. Hence, we fix
a potential race with the spi core where we could be left witn an
invalid 'max_speed_hz'.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Fixes: 5eda3550a3cc1 ("staging:iio:adis16400: Preallocate transfer message")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index b2f92b55b910..cb8d3ffab6fc 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -641,25 +641,13 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adis16400_state *st = iio_priv(indio_dev);
 	struct adis *adis = &st->adis;
-	u32 old_speed_hz = st->adis.spi->max_speed_hz;
 	void *buffer;
 	int ret;
 
-	if (!(st->variant->flags & ADIS16400_NO_BURST) &&
-		st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
-		st->adis.spi->max_speed_hz = ADIS16400_SPI_BURST;
-		spi_setup(st->adis.spi);
-	}
-
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret)
 		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
 
-	if (!(st->variant->flags & ADIS16400_NO_BURST)) {
-		st->adis.spi->max_speed_hz = old_speed_hz;
-		spi_setup(st->adis.spi);
-	}
-
 	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
 		buffer = adis->buffer + sizeof(u16);
 	else
@@ -965,7 +953,8 @@ static const char * const adis16400_status_error_msgs[] = {
 		BIT(ADIS16400_DIAG_STAT_POWER_LOW),			\
 	.timeouts = (_timeouts),					\
 	.burst_reg_cmd = ADIS16400_GLOB_CMD,				\
-	.burst_len = (_burst_len)					\
+	.burst_len = (_burst_len),					\
+	.burst_max_speed_hz = ADIS16400_SPI_BURST			\
 }
 
 static const struct adis_timeout adis16300_timeouts = {
-- 
2.31.1

Re: [PATCH v3 0/6] Adis IRQ fixes and minor improvements

[Jonathan Cameron]

On Tue, 27 Apr 2021 10:54:48 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> The primary goal of this series was to fix the return value on some
> trigger handlers as spotted in [1]. While doing it, I found some minor
> improvements that I think are simple enough to include in this series.
> 
> As for the first 2 patches, I opted to not do any functional change so
> I'm keeping the 'if (!adis->buffer)' check. However, 'adis-buffer' is
> allocated in 'update_scan_mode' hook which means we should be sure that
> the buffer is allocated and the check is really not needed. I did not
> went into the details but is there any way for the trigger handler to be
> called before the 'update_scan_mode' hook? If not, I'm happy in sending
> a v2 where we just remove the 'if'...
> 
> 
> Changes in v2:
>  * Remove the 'if' check for the allocated buffer;
>  * Make sure the adis 'state_lock' is unlocked on error paths;
>  * Fixed the commit message on the first 3 patches.
>  * Dropped ("iio: adis16475: re-set max spi transfer") and added 3 new
> patches (last 3 in the series ) to fix a potential race with the spi core
> as discussed in [2].
> 
> Changes in v3:
>  * Improved commit description on ("iio: adis16475: do not return ints in
> irq handlers");
>  * Reordered patch 2 and 3 so that patch 3 is easier to apply;
>  * Re-arranged the error handling on 'spi_sync()' error path.
> 
> [1]: https://marc.info/?l=linux-iio&m=161815197426882&w=2
> [2]: https://marc.info/?l=linux-iio&m=161884696722142&w=2

Not rushing these in as not comfortable with marking them for stable.
If we want to do a specific stable request at a later date that can
easily explain we need all of them.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to poke at them and see if they can find anything we
missed.

Thanks,

Jonathan

> 
> Nuno Sa (6):
>   iio: adis16475: do not return ints in irq handlers
>   iio: adis_buffer: update device page after changing it
>   iio: adis_buffer: don't push data to buffers on failure
>   iio: adis: add burst_max_speed_hz variable
>   iio: adis16475: do not directly change spi 'max_speed_hz'
>   iio: adis16400: do not directly change spi 'max_speed_hz'
> 
>  drivers/iio/imu/adis16400.c   | 15 ++-------------
>  drivers/iio/imu/adis16475.c   |  9 +++------
>  drivers/iio/imu/adis_buffer.c | 16 ++++++++++------
>  include/linux/iio/imu/adis.h  |  2 ++
>  4 files changed, 17 insertions(+), 25 deletions(-)
> 

Re: [PATCH v3 2/6] iio: adis_buffer: update device page after changing it

[Alexandru Ardelean]

On Tue, Apr 27, 2021 at 11:53 AM Nuno Sa <nuno.sa@analog.com> wrote:
>
> It makes more sense to update the device page as soon as we we
> successfully changed it. Moreover, a follow up patch will handle
> 'spi_sync' error path which would leave 'current_page' in a inconsistent
> state.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 0ae551a748eb..4fc0e0ca7561 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                                 mutex_unlock(&adis->state_lock);
>                                 goto irq_done;
>                         }
> +
> +                       adis->current_page = 0;
>                 }
>         }
>
> @@ -148,10 +150,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                 dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
>
>
> -       if (adis->data->has_paging) {
> -               adis->current_page = 0;
> +       if (adis->data->has_paging)
>                 mutex_unlock(&adis->state_lock);
> -       }
>
>         iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
>                 pf->timestamp);
> --
> 2.31.1
>