[PATCH 0/7] Adis IRQ fixes and minor improvements

[PATCH 0/7] 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'...

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

Nuno Sa (7):
  iio: adis_buffer: do not return ints in irq handlers
  iio: adis16400: do not return ints in irq handlers
  iio: adis16475: do not return ints in irq handlers
  iio: adis16475: re-set max spi transfer
  iio: adis_buffer: check return value on page change
  iio: adis_buffer: don't push data to buffers on failure
  iio: adis_buffer: update device page after changing it

 drivers/iio/imu/adis16400.c   |  3 ++-
 drivers/iio/imu/adis16475.c   |  6 ++++--
 drivers/iio/imu/adis_buffer.c | 20 +++++++++++++-------
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.31.1

[PATCH 1/7] iio: adis_buffer: do not return ints in irq handlers

[Nuno Sa]

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

Fixes: ccd2b52f4ac69 ("staging:iio: Add common ADIS library")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index ac354321f63a..f89bce10090a 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -130,7 +130,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	int ret;
 
 	if (!adis->buffer)
-		return -ENOMEM;
+		goto irq_done;
 
 	if (adis->data->has_paging) {
 		mutex_lock(&adis->state_lock);
@@ -154,6 +154,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
 		pf->timestamp);
 
+irq_done:
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
-- 
2.31.1

[PATCH 2/7] iio: adis16400: do not return ints in irq handlers

[Nuno Sa]

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

Fixes: 5eda3550a3cc1 ("staging:iio:adis16400: Preallocate transfer message")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 768aa493a1a6..c2362a9ee3c3 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -646,7 +646,7 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
 	int ret;
 
 	if (!adis->buffer)
-		return -ENOMEM;
+		goto irq_done;
 
 	if (!(st->variant->flags & ADIS16400_NO_BURST) &&
 		st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
@@ -671,6 +671,7 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
 	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
 		pf->timestamp);
 
+irq_done:
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
-- 
2.31.1

[PATCH 3/7] iio: adis16475: do not return ints in irq handlers

[Nuno Sa]

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

Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")

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 4/7] iio: adis16475: re-set max spi transfer

[Nuno Sa]

In case 'spi_sync()' fails, we would be left with a max spi transfer
which is not the one the user expects it to be. Hence, we need to re-set
it also in this error path.

Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 51b76444db0b..9dca7e506200 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -1067,8 +1067,10 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
 	adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
 
 	ret = spi_sync(adis->spi, &adis->msg);
-	if (ret)
+	if (ret) {
+		adis->spi->max_speed_hz = cached_spi_speed_hz;
 		goto check_burst32;
+	}
 
 	adis->spi->max_speed_hz = cached_spi_speed_hz;
 	buffer = adis->buffer;
-- 
2.31.1

[PATCH 5/7] iio: adis_buffer: check return value on page change

[Nuno Sa]

On the trigger handler, we might need to change the device page. Hence,
we should check the return value from 'spi_write()' and act accordingly.

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

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index f89bce10090a..7ab15c08889f 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -137,7 +137,11 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 		if (adis->current_page != 0) {
 			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
 			adis->tx[1] = 0;
-			spi_write(adis->spi, adis->tx, 2);
+			ret = spi_write(adis->spi, adis->tx, 2);
+			if (ret) {
+				dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
+				goto irq_done;
+			}
 		}
 	}
 
-- 
2.31.1

[PATCH 6/7] 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.

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

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 7ab15c08889f..73790b71102b 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -146,9 +146,10 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	}
 
 	ret = spi_sync(adis->spi, &adis->msg);
-	if (ret)
+	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
-
+		goto irq_done;
+	}
 
 	if (adis->data->has_paging) {
 		adis->current_page = 0;
-- 
2.31.1

[PATCH 7/7] iio: adis_buffer: update device page after changing it

[Nuno Sa]

With commit 41f2770a07e0 ("iio: adis_buffer: don't push data to buffers on
failure"), we return if 'spi_sync()' fails which would leave
'adis->current_page' in an incoherent state. Hence, set this variable
right after we change the device page.

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 73790b71102b..aa37981c28f1 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -142,6 +142,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 				dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
 				goto irq_done;
 			}
+
+			adis->current_page = 0;
 		}
 	}
 
@@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 		goto irq_done;
 	}
 
-	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

Re: [PATCH 1/7] iio: adis_buffer: do not return ints in irq handlers

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> On an IRQ handler we should return normal error codes as 'irqreturn_t'
> is expected.
>

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

> Fixes: ccd2b52f4ac69 ("staging:iio: Add common ADIS library")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index ac354321f63a..f89bce10090a 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -130,7 +130,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>         int ret;
>
>         if (!adis->buffer)
> -               return -ENOMEM;
> +               goto irq_done;
>
>         if (adis->data->has_paging) {
>                 mutex_lock(&adis->state_lock);
> @@ -154,6 +154,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>         iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
>                 pf->timestamp);
>
> +irq_done:
>         iio_trigger_notify_done(indio_dev->trig);
>
>         return IRQ_HANDLED;
> --
> 2.31.1
>

Re: [PATCH 2/7] iio: adis16400: do not return ints in irq handlers

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> On an IRQ handler we should return normal error codes as 'irqreturn_t'
> is expected.
>

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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 768aa493a1a6..c2362a9ee3c3 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -646,7 +646,7 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
>         int ret;
>
>         if (!adis->buffer)
> -               return -ENOMEM;
> +               goto irq_done;
>
>         if (!(st->variant->flags & ADIS16400_NO_BURST) &&
>                 st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
> @@ -671,6 +671,7 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
>         iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>                 pf->timestamp);
>
> +irq_done:
>         iio_trigger_notify_done(indio_dev->trig);
>
>         return IRQ_HANDLED;
> --
> 2.31.1
>

Re: [PATCH 3/7] iio: adis16475: do not return ints in irq handlers

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> On an IRQ handler we should return normal error codes as 'irqreturn_t'
> is expected.
>
> Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
>
> 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;

This is also going to call adis16475_burst32_check().
Which in itself is [probably] an undesired behavior change.

Maybe this needs a new 'irq_done' label?

>
>         adis->spi->max_speed_hz = cached_spi_speed_hz;
>         buffer = adis->buffer;
> --
> 2.31.1
>

Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> In case 'spi_sync()' fails, we would be left with a max spi transfer
> which is not the one the user expects it to be. Hence, we need to re-set
> it also in this error path.
>
> Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis16475.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 51b76444db0b..9dca7e506200 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -1067,8 +1067,10 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
>         adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
>
>         ret = spi_sync(adis->spi, &adis->msg);

Purely stylistic here.
But, the restore from the cached variable could be done here in a single line.
So. just moving [1] here.

> -       if (ret)
> +       if (ret) {
> +               adis->spi->max_speed_hz = cached_spi_speed_hz;
>                 goto check_burst32;
> +       }
>
>         adis->spi->max_speed_hz = cached_spi_speed_hz;
[1]

>         buffer = adis->buffer;
> --
> 2.31.1
>

Re: [PATCH 5/7] iio: adis_buffer: check return value on page change

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> On the trigger handler, we might need to change the device page. Hence,
> we should check the return value from 'spi_write()' and act accordingly.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index f89bce10090a..7ab15c08889f 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -137,7 +137,11 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                 if (adis->current_page != 0) {
>                         adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
>                         adis->tx[1] = 0;
> -                       spi_write(adis->spi, adis->tx, 2);
> +                       ret = spi_write(adis->spi, adis->tx, 2);
> +                       if (ret) {
> +                               dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
> +                               goto irq_done;
> +                       }

The &adis->state_lock is not being unlocked on this error path.
This probably needs a bit of re-organization of this block:
    if (adis->data->has_paging) {
        adis->current_page = 0;
        mutex_unlock(&adis->state_lock);
    }


>                 }
>         }
>
> --
> 2.31.1
>

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

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> There's no point in pushing data to IIO buffers in case 'spi_sync()'
> fails.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 7ab15c08889f..73790b71102b 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -146,9 +146,10 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>         }
>
>         ret = spi_sync(adis->spi, &adis->msg);
> -       if (ret)
> +       if (ret) {
>                 dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> -
> +               goto irq_done;
> +       }

This also has some issue with not unlocking the &adis->state_lock.

>
>         if (adis->data->has_paging) {
>                 adis->current_page = 0;
> --
> 2.31.1
>

Re: [PATCH 7/7] iio: adis_buffer: update device page after changing it

[Alexandru Ardelean]

On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> With commit 41f2770a07e0 ("iio: adis_buffer: don't push data to buffers on
> failure"), we return if 'spi_sync()' fails which would leave
> 'adis->current_page' in an incoherent state. Hence, set this variable
> right after we change the device page.
>
> 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 73790b71102b..aa37981c28f1 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -142,6 +142,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                                 dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
>                                 goto irq_done;
>                         }
> +
> +                       adis->current_page = 0;

If the above spi_write() fails, this adis->current_page isn't reset.
Maybe reset this as the first thing in this if block?

>                 }
>         }
>
> @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                 goto irq_done;
>         }
>
> -       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
>

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

[Lars-Peter Clausen]

On 4/13/21 1:20 PM, Nuno Sa 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'...

I do remember that the check was deliberate. I do remember of thinking 
about whether we need this and feeling uncomfortable about not having 
the check. But I think it is more a instance of defensive programming 
rather than actually being required.

It was less about the trigger being called before update_scan_mode, but 
in case it gets called if the allocation in update_scan_mode fails. But 
I think this should not be possible. So it is probably safe to remove it.

- Lars

RE: [PATCH 3/7] iio: adis16475: do not return ints in irq handlers

[Sa, Nuno]

> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, April 14, 2021 9:27 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 3/7] iio: adis16475: do not return ints in irq
> handlers
> 
> [External]
> 
> On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > On an IRQ handler we should return normal error codes as
> 'irqreturn_t'
> > is expected.
> >
> > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> >
> > 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;
> 
> This is also going to call adis16475_burst32_check().
> Which in itself is [probably] an undesired behavior change.
> 
> Maybe this needs a new 'irq_done' label?

That was intentional. If someone changed the decimation or the FIR
filters so that we can change to burst32, we can just do it now...

RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer

[Sa, Nuno]

> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, April 14, 2021 9:29 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> 
> [External]
> 
> On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > In case 'spi_sync()' fails, we would be left with a max spi transfer
> > which is not the one the user expects it to be. Hence, we need to re-
> set
> > it also in this error path.
> >
> > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis16475.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> > index 51b76444db0b..9dca7e506200 100644
> > --- a/drivers/iio/imu/adis16475.c
> > +++ b/drivers/iio/imu/adis16475.c
> > @@ -1067,8 +1067,10 @@ static irqreturn_t
> adis16475_trigger_handler(int irq, void *p)
> >         adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
> >
> >         ret = spi_sync(adis->spi, &adis->msg);
> 
> Purely stylistic here.
> But, the restore from the cached variable could be done here in a
> single line.
> So. just moving [1] here.

You mean also doing it in the label? I thought about that and the reason
why I didn't is that on a normal run, I want to reset the max freq as soon
as possible so that if someone concurrently tries to read 'direct mode' attrs
gets the max freq. This was my reasoning but I admit that it's not that
important so I will leave this to Jonathan's preference...

Hmm now that I spoke about the concurrently access to IIO attr and being paranoid about
the compiler, I wonder if we should not use
WRITE_ONCE(adis->spi->max_speed_hz, ADIS16475_BURST_MAX_SPEED)...

RE: [PATCH 5/7] iio: adis_buffer: check return value on page change

[Sa, Nuno]

> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, April 14, 2021 9:34 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 5/7] iio: adis_buffer: check return value on page
> change
> 
> [External]
> 
> On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > On the trigger handler, we might need to change the device page.
> Hence,
> > we should check the return value from 'spi_write()' and act
> accordingly.
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis_buffer.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> > index f89bce10090a..7ab15c08889f 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -137,7 +137,11 @@ static irqreturn_t adis_trigger_handler(int
> irq, void *p)
> >                 if (adis->current_page != 0) {
> >                         adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> >                         adis->tx[1] = 0;
> > -                       spi_write(adis->spi, adis->tx, 2);
> > +                       ret = spi_write(adis->spi, adis->tx, 2);
> > +                       if (ret) {
> > +                               dev_err(&adis->spi->dev, "Failed to change device
> page: %d\n", ret);
> > +                               goto irq_done;
> > +                       }
> 
> The &adis->state_lock is not being unlocked on this error path.
> This probably needs a bit of re-organization of this block:
>     if (adis->data->has_paging) {
>         adis->current_page = 0;
>         mutex_unlock(&adis->state_lock);
>     }
> 

Arghhh, good catch. Not sure where I had my head to not see this...

Thanks!

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

[Sa, Nuno]

> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, April 14, 2021 9:35 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 6/7] iio: adis_buffer: don't push data to buffers on
> failure
> 
> [External]
> 
> On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > There's no point in pushing data to IIO buffers in case 'spi_sync()'
> > fails.
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis_buffer.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> > index 7ab15c08889f..73790b71102b 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -146,9 +146,10 @@ static irqreturn_t adis_trigger_handler(int
> irq, void *p)
> >         }
> >
> >         ret = spi_sync(adis->spi, &adis->msg);
> > -       if (ret)
> > +       if (ret) {
> >                 dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> > -
> > +               goto irq_done;
> > +       }
> 
> This also has some issue with not unlocking the &adis->state_lock.

Ditto :facepalm:

Thanks!

RE: [PATCH 7/7] iio: adis_buffer: update device page after changing it

[Sa, Nuno]

> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, April 14, 2021 9:39 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 7/7] iio: adis_buffer: update device page after
> changing it
> 
> [External]
> 
> On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > With commit 41f2770a07e0 ("iio: adis_buffer: don't push data to
> buffers on
> > failure"), we return if 'spi_sync()' fails which would leave
> > 'adis->current_page' in an incoherent state. Hence, set this variable
> > right after we change the device page.
> >
> > 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 73790b71102b..aa37981c28f1 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -142,6 +142,8 @@ static irqreturn_t adis_trigger_handler(int irq,
> void *p)
> >                                 dev_err(&adis->spi->dev, "Failed to change device
> page: %d\n", ret);
> >                                 goto irq_done;
> >                         }
> > +
> > +                       adis->current_page = 0;
> 
> If the above spi_write() fails, this adis->current_page isn't reset.
> Maybe reset this as the first thing in this if block?
> 

If the 'spi_write()' fails it means that we could not change to page0,
so we should not update the current_page...

RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer

[Sa, Nuno]

> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Thursday, April 15, 2021 9:54 AM
> To: Alexandru Ardelean <ardeleanalex@gmail.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> 
> [External]
> 
> 
> 
> > -----Original Message-----
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Wednesday, April 14, 2021 9:29 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> >
> > [External]
> >
> > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> > wrote:
> > >
> > > In case 'spi_sync()' fails, we would be left with a max spi transfer
> > > which is not the one the user expects it to be. Hence, we need to
> re-
> > set
> > > it also in this error path.
> > >
> > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/imu/adis16475.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/imu/adis16475.c
> b/drivers/iio/imu/adis16475.c
> > > index 51b76444db0b..9dca7e506200 100644
> > > --- a/drivers/iio/imu/adis16475.c
> > > +++ b/drivers/iio/imu/adis16475.c
> > > @@ -1067,8 +1067,10 @@ static irqreturn_t
> > adis16475_trigger_handler(int irq, void *p)
> > >         adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
> > >
> > >         ret = spi_sync(adis->spi, &adis->msg);
> >
> > Purely stylistic here.
> > But, the restore from the cached variable could be done here in a
> > single line.
> > So. just moving [1] here.
> 
> You mean also doing it in the label? I thought about that and the
> reason
> why I didn't is that on a normal run, I want to reset the max freq as
> soon
> as possible so that if someone concurrently tries to read 'direct mode'
> attrs
> gets the max freq. This was my reasoning but I admit that it's not that
> important so I will leave this to Jonathan's preference...
> 
> Hmm now that I spoke about the concurrently access to IIO attr and
> being paranoid about
> the compiler, I wonder if we should not use
> WRITE_ONCE(adis->spi->max_speed_hz,
> ADIS16475_BURST_MAX_SPEED)...

Hmmm, actually WRITE_ONCE would not be any help since the spi core
does not use READ_ONCE. So, if we are going to be paranoid about the
compiler and load/store tearing, I guess the only safe way here is to
acquire the adis lock [btw, I'm a bit paranoid with this stuff :)]...

Anyways, arguably the likelihood for this to happen is really, really small... 

Re: [PATCH 3/7] iio: adis16475: do not return ints in irq handlers

[Alexandru Ardelean]

On Thu, Apr 15, 2021 at 10:38 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
>
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Wednesday, April 14, 2021 9:27 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: Re: [PATCH 3/7] iio: adis16475: do not return ints in irq
> > handlers
> >
> > [External]
> >
> > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> > wrote:
> > >
> > > On an IRQ handler we should return normal error codes as
> > 'irqreturn_t'
> > > is expected.
> > >
> > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > >
> > > 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;
> >
> > This is also going to call adis16475_burst32_check().
> > Which in itself is [probably] an undesired behavior change.
> >
> > Maybe this needs a new 'irq_done' label?
>
> That was intentional. If someone changed the decimation or the FIR
> filters so that we can change to burst32, we can just do it now...
>

ack

Re: [PATCH 7/7] iio: adis_buffer: update device page after changing it

[Alexandru Ardelean]

On Thu, Apr 15, 2021 at 10:58 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Wednesday, April 14, 2021 9:39 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: Re: [PATCH 7/7] iio: adis_buffer: update device page after
> > changing it
> >
> > [External]
> >
> > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> > wrote:
> > >
> > > With commit 41f2770a07e0 ("iio: adis_buffer: don't push data to
> > buffers on
> > > failure"), we return if 'spi_sync()' fails which would leave
> > > 'adis->current_page' in an incoherent state. Hence, set this variable
> > > right after we change the device page.
> > >
> > > 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 73790b71102b..aa37981c28f1 100644
> > > --- a/drivers/iio/imu/adis_buffer.c
> > > +++ b/drivers/iio/imu/adis_buffer.c
> > > @@ -142,6 +142,8 @@ static irqreturn_t adis_trigger_handler(int irq,
> > void *p)
> > >                                 dev_err(&adis->spi->dev, "Failed to change device
> > page: %d\n", ret);
> > >                                 goto irq_done;
> > >                         }
> > > +
> > > +                       adis->current_page = 0;
> >
> > If the above spi_write() fails, this adis->current_page isn't reset.
> > Maybe reset this as the first thing in this if block?
> >
>
> If the 'spi_write()' fails it means that we could not change to page0,
> so we should not update the current_page...

ack

Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer

[Jonathan Cameron]

On Thu, 15 Apr 2021 08:16:30 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Sa, Nuno <Nuno.Sa@analog.com>
> > Sent: Thursday, April 15, 2021 9:54 AM
> > To: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > 
> > [External]
> > 
> > 
> >   
> > > -----Original Message-----
> > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Sent: Wednesday, April 14, 2021 9:29 AM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > <jic23@kernel.org>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > <lars@metafoo.de>
> > > Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > >
> > > [External]
> > >
> > > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> > > wrote:  
> > > >
> > > > In case 'spi_sync()' fails, we would be left with a max spi transfer
> > > > which is not the one the user expects it to be. Hence, we need to  
> > re-  
> > > set  
> > > > it also in this error path.
> > > >
> > > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  drivers/iio/imu/adis16475.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iio/imu/adis16475.c  
> > b/drivers/iio/imu/adis16475.c  
> > > > index 51b76444db0b..9dca7e506200 100644
> > > > --- a/drivers/iio/imu/adis16475.c
> > > > +++ b/drivers/iio/imu/adis16475.c
> > > > @@ -1067,8 +1067,10 @@ static irqreturn_t  
> > > adis16475_trigger_handler(int irq, void *p)  
> > > >         adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
> > > >
> > > >         ret = spi_sync(adis->spi, &adis->msg);  
> > >
> > > Purely stylistic here.
> > > But, the restore from the cached variable could be done here in a
> > > single line.
> > > So. just moving [1] here.  
> > 
> > You mean also doing it in the label? I thought about that and the
> > reason
> > why I didn't is that on a normal run, I want to reset the max freq as
> > soon
> > as possible so that if someone concurrently tries to read 'direct mode'
> > attrs
> > gets the max freq. This was my reasoning but I admit that it's not that
> > important so I will leave this to Jonathan's preference...
> > 
> > Hmm now that I spoke about the concurrently access to IIO attr and
> > being paranoid about
> > the compiler, I wonder if we should not use
> > WRITE_ONCE(adis->spi->max_speed_hz,
> > ADIS16475_BURST_MAX_SPEED)...  
> 
> Hmmm, actually WRITE_ONCE would not be any help since the spi core
> does not use READ_ONCE. So, if we are going to be paranoid about the
> compiler and load/store tearing, I guess the only safe way here is to
> acquire the adis lock [btw, I'm a bit paranoid with this stuff :)]...
> 
> Anyways, arguably the likelihood for this to happen is really, really small... 

Really small, but needs fixing.  We shouldn't have a window in which this
can happen.  So either we need to stop those attributes from reading whilst
we are in buffered mode (via claim_direct_mode pattern) or we need to put
a lock around this.  As an alternative, could we use the speed_hz field
in appropriate spi_transfer structures to tweak in this path without
affecting others?  That should make this concurrency problem an issue
for the spi core (which I'd assume handles this).

I'm going to hold this series for now on basis we should resolve this whilst
here.

Jonathan

RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, April 18, 2021 12:21 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Alexandru Ardelean <ardeleanalex@gmail.com>; linux-iio <linux-
> iio@vger.kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> 
> [External]
> 
> On Thu, 15 Apr 2021 08:16:30 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > Sent: Thursday, April 15, 2021 9:54 AM
> > > To: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > <jic23@kernel.org>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > <lars@metafoo.de>
> > > Subject: RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > >
> > > [External]
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > Sent: Wednesday, April 14, 2021 9:29 AM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > <jic23@kernel.org>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > <lars@metafoo.de>
> > > > Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > > >
> > > > [External]
> > > >
> > > > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> > > > wrote:
> > > > >
> > > > > In case 'spi_sync()' fails, we would be left with a max spi
> transfer
> > > > > which is not the one the user expects it to be. Hence, we need
> to
> > > re-
> > > > set
> > > > > it also in this error path.
> > > > >
> > > > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > ---
> > > > >  drivers/iio/imu/adis16475.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/iio/imu/adis16475.c
> > > b/drivers/iio/imu/adis16475.c
> > > > > index 51b76444db0b..9dca7e506200 100644
> > > > > --- a/drivers/iio/imu/adis16475.c
> > > > > +++ b/drivers/iio/imu/adis16475.c
> > > > > @@ -1067,8 +1067,10 @@ static irqreturn_t
> > > > adis16475_trigger_handler(int irq, void *p)
> > > > >         adis->spi->max_speed_hz =
> ADIS16475_BURST_MAX_SPEED;
> > > > >
> > > > >         ret = spi_sync(adis->spi, &adis->msg);
> > > >
> > > > Purely stylistic here.
> > > > But, the restore from the cached variable could be done here in a
> > > > single line.
> > > > So. just moving [1] here.
> > >
> > > You mean also doing it in the label? I thought about that and the
> > > reason
> > > why I didn't is that on a normal run, I want to reset the max freq as
> > > soon
> > > as possible so that if someone concurrently tries to read 'direct
> mode'
> > > attrs
> > > gets the max freq. This was my reasoning but I admit that it's not
> that
> > > important so I will leave this to Jonathan's preference...
> > >
> > > Hmm now that I spoke about the concurrently access to IIO attr
> and
> > > being paranoid about
> > > the compiler, I wonder if we should not use
> > > WRITE_ONCE(adis->spi->max_speed_hz,
> > > ADIS16475_BURST_MAX_SPEED)...
> >
> > Hmmm, actually WRITE_ONCE would not be any help since the spi
> core
> > does not use READ_ONCE. So, if we are going to be paranoid about
> the
> > compiler and load/store tearing, I guess the only safe way here is to
> > acquire the adis lock [btw, I'm a bit paranoid with this stuff :)]...
> >
> > Anyways, arguably the likelihood for this to happen is really, really
> small...
> 
> Really small, but needs fixing.  We shouldn't have a window in which
> this

Agreed!

> can happen.  So either we need to stop those attributes from reading
> whilst
> we are in buffered mode (via claim_direct_mode pattern) or we need
> to put
> a lock around this.  As an alternative, could we use the speed_hz field
> in appropriate spi_transfer structures to tweak in this path without
> affecting others?  That should make this concurrency problem an issue
> for the spi core (which I'd assume handles this).

Hmm, I like the 'speed_hz' approach as there's no reason to grab
the lock because of a spi core tweak. As you said, with this, we push things
to spi core. Going one step further, I think the most appropriate thing to do
is actually come up with a new member in the 'adis_data' struct
(like burst_max_speed_hz) and tweak the burst mode transfers accordingly in
'adis_update_scan_mode_burst()' (as there's no need to set this on every
burst sample)...

If we are going the above path, what would be your preference? To add the
patches to this series? Or to just fix this patch and [1] and push another series
with the above changes? Hmm, since [1] would also depend on this, I guess the
later approach would be better?

[1]: https://patchwork.kernel.org/project/linux-iio/patch/20210413092815.28626-1-nuno.sa@analog.com/

- Nuno Sá

Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer

[Jonathan Cameron]

On Mon, 19 Apr 2021 07:47:55 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, April 18, 2021 12:21 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Alexandru Ardelean <ardeleanalex@gmail.com>; linux-iio <linux-  
> > iio@vger.kernel.org>; Hennerich, Michael  
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > 
> > [External]
> > 
> > On Thu, 15 Apr 2021 08:16:30 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Sent: Thursday, April 15, 2021 9:54 AM
> > > > To: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > <jic23@kernel.org>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > <lars@metafoo.de>
> > > > Subject: RE: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > > >
> > > > [External]
> > > >
> > > >
> > > >  
> > > > > -----Original Message-----
> > > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > > Sent: Wednesday, April 14, 2021 9:29 AM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > > <jic23@kernel.org>; Hennerich, Michael
> > > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > > <lars@metafoo.de>
> > > > > Subject: Re: [PATCH 4/7] iio: adis16475: re-set max spi transfer
> > > > >
> > > > > [External]
> > > > >
> > > > > On Tue, Apr 13, 2021 at 5:45 PM Nuno Sa <nuno.sa@analog.com>
> > > > > wrote:  
> > > > > >
> > > > > > In case 'spi_sync()' fails, we would be left with a max spi  
> > transfer  
> > > > > > which is not the one the user expects it to be. Hence, we need  
> > to  
> > > > re-  
> > > > > set  
> > > > > > it also in this error path.
> > > > > >
> > > > > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > > ---
> > > > > >  drivers/iio/imu/adis16475.c | 4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/imu/adis16475.c  
> > > > b/drivers/iio/imu/adis16475.c  
> > > > > > index 51b76444db0b..9dca7e506200 100644
> > > > > > --- a/drivers/iio/imu/adis16475.c
> > > > > > +++ b/drivers/iio/imu/adis16475.c
> > > > > > @@ -1067,8 +1067,10 @@ static irqreturn_t  
> > > > > adis16475_trigger_handler(int irq, void *p)  
> > > > > >         adis->spi->max_speed_hz =  
> > ADIS16475_BURST_MAX_SPEED;  
> > > > > >
> > > > > >         ret = spi_sync(adis->spi, &adis->msg);  
> > > > >
> > > > > Purely stylistic here.
> > > > > But, the restore from the cached variable could be done here in a
> > > > > single line.
> > > > > So. just moving [1] here.  
> > > >
> > > > You mean also doing it in the label? I thought about that and the
> > > > reason
> > > > why I didn't is that on a normal run, I want to reset the max freq as
> > > > soon
> > > > as possible so that if someone concurrently tries to read 'direct  
> > mode'  
> > > > attrs
> > > > gets the max freq. This was my reasoning but I admit that it's not  
> > that  
> > > > important so I will leave this to Jonathan's preference...
> > > >
> > > > Hmm now that I spoke about the concurrently access to IIO attr  
> > and  
> > > > being paranoid about
> > > > the compiler, I wonder if we should not use
> > > > WRITE_ONCE(adis->spi->max_speed_hz,
> > > > ADIS16475_BURST_MAX_SPEED)...  
> > >
> > > Hmmm, actually WRITE_ONCE would not be any help since the spi  
> > core  
> > > does not use READ_ONCE. So, if we are going to be paranoid about  
> > the  
> > > compiler and load/store tearing, I guess the only safe way here is to
> > > acquire the adis lock [btw, I'm a bit paranoid with this stuff :)]...
> > >
> > > Anyways, arguably the likelihood for this to happen is really, really  
> > small...
> > 
> > Really small, but needs fixing.  We shouldn't have a window in which
> > this  
> 
> Agreed!
> 
> > can happen.  So either we need to stop those attributes from reading
> > whilst
> > we are in buffered mode (via claim_direct_mode pattern) or we need
> > to put
> > a lock around this.  As an alternative, could we use the speed_hz field
> > in appropriate spi_transfer structures to tweak in this path without
> > affecting others?  That should make this concurrency problem an issue
> > for the spi core (which I'd assume handles this).  
> 
> Hmm, I like the 'speed_hz' approach as there's no reason to grab
> the lock because of a spi core tweak. As you said, with this, we push things
> to spi core. Going one step further, I think the most appropriate thing to do
> is actually come up with a new member in the 'adis_data' struct
> (like burst_max_speed_hz) and tweak the burst mode transfers accordingly in
> 'adis_update_scan_mode_burst()' (as there's no need to set this on every
> burst sample)...
> 
> If we are going the above path, what would be your preference? To add the
> patches to this series? Or to just fix this patch and [1] and push another series
> with the above changes? Hmm, since [1] would also depend on this, I guess the
> later approach would be better?
> 
> [1]: https://patchwork.kernel.org/project/linux-iio/patch/20210413092815.28626-1-nuno.sa@analog.com/

Make a new version of this series with the fix, then provide an update of [1]
that is probably going to be dependent on this series.

Given this is a fix, ideally we'll backport it which we don't need to do
for the new code introduced in [1].

Thanks,

Jonathan

> 
> - Nuno Sá