[PATCH] staging: iio: adc: ad7816: add timeout to busy-wait loop

[PATCH] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Ruslan Valiyev]

The ad7816_spi_read() function polls the busy GPIO pin in a tight
loop without any timeout. If the hardware fails to deassert the
busy signal, the kernel hangs indefinitely in an unbounded
busy-wait.

Replace the open-coded while/cpu_relax() loop with
read_poll_timeout() which polls every 5 us and returns -ETIMEDOUT
after 1 ms. Per the AD7816 datasheet the maximum conversion time
is 27 us (temperature channel), so 1 ms provides generous margin.

Also handle the case where gpiod_get_value() returns a negative
error code, which the original loop silently treated as not-busy.

Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 172acf135..7b8a78e4d 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/gpio/consumer.h>
 #include <linux/device.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -85,8 +86,14 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	}
 
 	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
-		while (gpiod_get_value(chip->busy_pin))
-			cpu_relax();
+		int val;
+
+		ret = read_poll_timeout(gpiod_get_value, val, val <= 0,
+					5, 1000, false, chip->busy_pin);
+		if (val < 0)
+			return val;
+		if (ret)
+			return ret;
 	}
 
 	gpiod_set_value(chip->rdwr_pin, 0);
-- 
2.43.0

Re: [PATCH] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Andy Shevchenko]

On Thu, Feb 26, 2026 at 05:26:15AM +0000, Ruslan Valiyev wrote:
> The ad7816_spi_read() function polls the busy GPIO pin in a tight
> loop without any timeout. If the hardware fails to deassert the
> busy signal, the kernel hangs indefinitely in an unbounded
> busy-wait.
> 
> Replace the open-coded while/cpu_relax() loop with
> read_poll_timeout() which polls every 5 us and returns -ETIMEDOUT
> after 1 ms. Per the AD7816 datasheet the maximum conversion time
> is 27 us (temperature channel), so 1 ms provides generous margin.
> 
> Also handle the case where gpiod_get_value() returns a negative
> error code, which the original loop silently treated as not-busy.

...

> -		while (gpiod_get_value(chip->busy_pin))
> -			cpu_relax();
> +		int val;
> +
> +		ret = read_poll_timeout(gpiod_get_value, val, val <= 0,
> +					5, 1000, false, chip->busy_pin);
> +		if (val < 0)
> +			return val;
> +		if (ret)
> +			return ret;

Have you able to test this?

Have you investigated the code? It was an atomic, now it's sleeping.
This is a huge behavioural change. See my first Q.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Ruslan Valiyev]

On Thu, Feb 26, 2026 at 05:26:15AM +0000, Andy Shevchenko wrote:
> Have you able to test this?

No, I don't have AD7816 hardware, compile-tested only.

> Have you investigated the code? It was an atomic, now it's sleeping.
> This is a huge behavioural change. See my first Q.

You're right, the sleeping change was unnecessary. ad7816_spi_read()
is only called from ad7816_show_value(), a sysfs show callback, so
it runs in process context. But for a wait of at most 27 us per the
datasheet, there's no benefit to sleeping. The atomic variant adds
the timeout protection while preserving the original busy-wait
semantics.

I've respun as v2 using read_poll_timeout_atomic(). Also fixed the
commit message: v1 incorrectly said the original loop treated
negative gpiod_get_value() errors as "not-busy", it's the opposite,
negative is non-zero so the loop treats it as busy and spins forever.

Ruslan

Re: [PATCH] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Dan Carpenter]

On Thu, Feb 26, 2026 at 08:25:35AM +0000, Ruslan Valiyev wrote:
> On Thu, Feb 26, 2026 at 05:26:15AM +0000, Andy Shevchenko wrote:
> > Have you able to test this?
> 
> No, I don't have AD7816 hardware, compile-tested only.
> 
> > Have you investigated the code? It was an atomic, now it's sleeping.
> > This is a huge behavioural change. See my first Q.
> 
> You're right, the sleeping change was unnecessary. ad7816_spi_read()
> is only called from ad7816_show_value(), a sysfs show callback, so
> it runs in process context. But for a wait of at most 27 us per the
> datasheet, there's no benefit to sleeping. The atomic variant adds
> the timeout protection while preserving the original busy-wait
> semantics.
> 

So this is only from reading the datasheet and the code?  I had assumed
this was something you experienced yourself in real life.

regards,
dan carpenter

[PATCH v2] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Ruslan Valiyev]

The ad7816_spi_read() function polls the busy GPIO pin in a tight
loop without any timeout. If the hardware fails to deassert the
busy signal, the kernel hangs indefinitely in an unbounded
busy-wait.

Replace the open-coded while/cpu_relax() loop with
read_poll_timeout_atomic() which polls every 5 us and returns
-ETIMEDOUT after 1 ms. Per the AD7816 datasheet the maximum
conversion time is 27 us (temperature channel), so 1 ms provides
generous margin.

Use the atomic variant to preserve the existing busy-wait semantics
of the original loop.

Also handle the case where gpiod_get_value() returns a negative
error code, which the original loop would treat as busy and keep
spinning on indefinitely.

Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 172acf135..9b4ae3ee2 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/gpio/consumer.h>
 #include <linux/device.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -85,8 +86,14 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	}
 
 	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
-		while (gpiod_get_value(chip->busy_pin))
-			cpu_relax();
+		int val;
+
+		ret = read_poll_timeout_atomic(gpiod_get_value, val, val <= 0,
+					5, 1000, false, chip->busy_pin);
+		if (val < 0)
+			return val;
+		if (ret)
+			return ret;
 	}
 
 	gpiod_set_value(chip->rdwr_pin, 0);
-- 
2.43.0

Re: [PATCH v2] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Andy Shevchenko]

On Thu, Feb 26, 2026 at 08:25:47AM +0000, Ruslan Valiyev wrote:
> The ad7816_spi_read() function polls the busy GPIO pin in a tight
> loop without any timeout. If the hardware fails to deassert the
> busy signal, the kernel hangs indefinitely in an unbounded
> busy-wait.
> 
> Replace the open-coded while/cpu_relax() loop with
> read_poll_timeout_atomic() which polls every 5 us and returns
> -ETIMEDOUT after 1 ms. Per the AD7816 datasheet the maximum
> conversion time is 27 us (temperature channel), so 1 ms provides
> generous margin.
> 
> Use the atomic variant to preserve the existing busy-wait semantics
> of the original loop.
> 
> Also handle the case where gpiod_get_value() returns a negative
> error code, which the original loop would treat as busy and keep
> spinning on indefinitely.

First of all, start a new email thread for a new version of the patch.

Second, give at least 24h between the versions to have a chance being
reviewed by more people.


>  	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> +		int val;
> +
> +		ret = read_poll_timeout_atomic(gpiod_get_value, val, val <= 0,

While val <= 0 is technically correct, semantically it's harder to read,
it's better to use val < 0 || val == 0.

> +					5, 1000, false, chip->busy_pin);

This has an indentation issue now.

> +		if (val < 0)
> +			return val;
> +		if (ret)
> +			return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: iio: adc: ad7816: add timeout to busy-wait loop

[Ruslan Valiyev]

On Thu, Feb 26, 2026, Dan Carpenter wrote:
> So this is only from reading the datasheet and the code?  I had assumed
> this was something you experienced yourself in real life.

Yes, purely from the datasheet and code analysis.

Ruslan