drivers/i2c/busses/i2c-ocores.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
The polling task can be preempted at any point inside ocores_wait(),
including just before the time_after() check. If the scheduler does
not resume the task until after the 1ms deadline, ocores_wait()
returns -ETIMEDOUT even though the hardware already cleared the
status bit.
Re-read the status register after a timeout before declaring failure.
This avoids spurious timeout warnings under high CPU load.
Signed-off-by: Martin Aberer <martin.aberer@bachmann.info>
---
drivers/i2c/busses/i2c-ocores.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 0f67e57cdeff..6f5aece94d57 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -313,12 +313,24 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
/*
* once we are here we expect to get the expected result immediately
* so if after 1ms we timeout then something is broken.
+ *
+ * The polling task can be preempted at any point inside ocores_wait(),
+ * including just before the time_after() check. If the scheduler does
+ * not resume the task until after the 1ms deadline, ocores_wait()
+ * returns -ETIMEDOUT even though the hardware already cleared the
+ * status bit.
+
+ * Re-read the status register after a timeout before declaring failure.
+ * This avoids spurious timeout warnings under high CPU load.
*/
err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
- if (err)
+ if (err) {
+ if ((oc_getreg(i2c, OCI2C_STATUS) & mask) == 0)
+ return 0;
dev_warn(i2c->adap.dev.parent,
"%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
__func__, mask);
+ }
return err;
}
--
2.43.0Hey Martin
> The polling task can be preempted at any point inside ocores_wait(),
> including just before the time_after() check. If the scheduler does
> not resume the task until after the 1ms deadline, ocores_wait()
> returns -ETIMEDOUT even though the hardware already cleared the
> status bit.
>
> Re-read the status register after a timeout before declaring failure.
> This avoids spurious timeout warnings under high CPU load.
>
> Signed-off-by: Martin Aberer <martin.aberer@bachmann.info>
> ---
> drivers/i2c/busses/i2c-ocores.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 0f67e57cdeff..6f5aece94d57 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -313,12 +313,24 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
> /*
> * once we are here we expect to get the expected result immediately
> * so if after 1ms we timeout then something is broken.
> + *
> + * The polling task can be preempted at any point inside ocores_wait(),
> + * including just before the time_after() check. If the scheduler does
> + * not resume the task until after the 1ms deadline, ocores_wait()
> + * returns -ETIMEDOUT even though the hardware already cleared the
> + * status bit.
> +
> + * Re-read the status register after a timeout before declaring failure.
> + * This avoids spurious timeout warnings under high CPU load.
> */
> err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
> - if (err)
> + if (err) {
> + if ((oc_getreg(i2c, OCI2C_STATUS) & mask) == 0)
> + return 0;
> dev_warn(i2c->adap.dev.parent,
> "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
> __func__, mask);
> + }
> return err;
> }
>
In my view, a proper fix would be to switch to read_poll_timeout_atomic().
Something like this - untested:
---8<----
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 0f67e57cdeff..df6ebf32d6e8 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -24,6 +24,7 @@
#include <linux/io.h>
#include <linux/log2.h>
#include <linux/spinlock.h>
+#include <linux/iopoll.h>
#include <linux/jiffies.h>
/*
@@ -258,7 +259,7 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
* @reg: register to query
* @mask: bitmask to apply on register value
* @val: expected result
- * @timeout: timeout in jiffies
+ * @timeout_us: timeout in microseconds
*
* Timeout is necessary to avoid to stay here forever when the chip
* does not answer correctly.
@@ -267,21 +268,14 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
*/
static int ocores_wait(struct ocores_i2c *i2c,
int reg, u8 mask, u8 val,
- const unsigned long timeout)
+ unsigned long timeout_us)
{
- unsigned long j;
+ u8 status;
- j = jiffies + timeout;
- while (1) {
- u8 status = oc_getreg(i2c, reg);
-
- if ((status & mask) == val)
- break;
-
- if (time_after(jiffies, j))
- return -ETIMEDOUT;
- }
- return 0;
+ return read_poll_timeout_atomic(oc_getreg, status,
+ (status & mask) == val,
+ 0, timeout_us, false,
+ i2c, reg);
}
/**
@@ -314,7 +308,7 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
* once we are here we expect to get the expected result immediately
* so if after 1ms we timeout then something is broken.
*/
- err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
+ err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, 1000);
if (err)
dev_warn(i2c->adap.dev.parent,
"%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
---8<---
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
On Mon, Mar 23, 2026 at 03:34:24PM +0100, Christian Gmeiner wrote:
> Hey Martin
>
> > The polling task can be preempted at any point inside ocores_wait(),
> > including just before the time_after() check. If the scheduler does
> > not resume the task until after the 1ms deadline, ocores_wait()
> > returns -ETIMEDOUT even though the hardware already cleared the
> > status bit.
> >
> > Re-read the status register after a timeout before declaring failure.
> > This avoids spurious timeout warnings under high CPU load.
> >
> > Signed-off-by: Martin Aberer <martin.aberer@bachmann.info>
> > ---
> > drivers/i2c/busses/i2c-ocores.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> > index 0f67e57cdeff..6f5aece94d57 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -313,12 +313,24 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
> > /*
> > * once we are here we expect to get the expected result immediately
> > * so if after 1ms we timeout then something is broken.
> > + *
> > + * The polling task can be preempted at any point inside ocores_wait(),
> > + * including just before the time_after() check. If the scheduler does
> > + * not resume the task until after the 1ms deadline, ocores_wait()
> > + * returns -ETIMEDOUT even though the hardware already cleared the
> > + * status bit.
> > +
> > + * Re-read the status register after a timeout before declaring failure.
> > + * This avoids spurious timeout warnings under high CPU load.
> > */
> > err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
> > - if (err)
> > + if (err) {
> > + if ((oc_getreg(i2c, OCI2C_STATUS) & mask) == 0)
> > + return 0;
> > dev_warn(i2c->adap.dev.parent,
> > "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
> > __func__, mask);
> > + }
> > return err;
> > }
> >
>
> In my view, a proper fix would be to switch to read_poll_timeout_atomic().
> Something like this - untested:
>
> ---8<----
>
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 0f67e57cdeff..df6ebf32d6e8 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -24,6 +24,7 @@
> #include <linux/io.h>
> #include <linux/log2.h>
> #include <linux/spinlock.h>
> +#include <linux/iopoll.h>
> #include <linux/jiffies.h>
> /*
> @@ -258,7 +259,7 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
> * @reg: register to query
> * @mask: bitmask to apply on register value
> * @val: expected result
> - * @timeout: timeout in jiffies
> + * @timeout_us: timeout in microseconds
> *
> * Timeout is necessary to avoid to stay here forever when the chip
> * does not answer correctly.
> @@ -267,21 +268,14 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
> */
> static int ocores_wait(struct ocores_i2c *i2c,
> int reg, u8 mask, u8 val,
> - const unsigned long timeout)
> + unsigned long timeout_us)
> {
> - unsigned long j;
> + u8 status;
Hi Christian
It looks like your email client has corrupted the patch, messing up
the white space.
Please post a real patch.
FYI: I agree iopoll.h is the way to go, i keep pointing out this class
a bugs in various places and always point developers at those macros.
Andrew
Replace the manual polling loop in ocores_wait() with the kernel helper
read_poll_timeout_atomic(). This simplifies the code and ensures robust
timeout handling. By using this helper, we avoid spurious timeout errors
that could occur under high CPU load or preemption, as the macro handles
timing and condition checks atomically.
Signed-off-by: Martin Aberer <martin.aberer@bachmann.info>
---
drivers/i2c/busses/i2c-ocores.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 0f67e57cdeff..df6ebf32d6e8 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -24,6 +24,7 @@
#include <linux/io.h>
#include <linux/log2.h>
#include <linux/spinlock.h>
+#include <linux/iopoll.h>
#include <linux/jiffies.h>
/*
@@ -258,7 +259,7 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
* @reg: register to query
* @mask: bitmask to apply on register value
* @val: expected result
- * @timeout: timeout in jiffies
+ * @timeout_us: timeout in microseconds
*
* Timeout is necessary to avoid to stay here forever when the chip
* does not answer correctly.
@@ -267,21 +268,14 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
*/
static int ocores_wait(struct ocores_i2c *i2c,
int reg, u8 mask, u8 val,
- const unsigned long timeout)
+ unsigned long timeout_us)
{
- unsigned long j;
+ u8 status;
- j = jiffies + timeout;
- while (1) {
- u8 status = oc_getreg(i2c, reg);
-
- if ((status & mask) == val)
- break;
-
- if (time_after(jiffies, j))
- return -ETIMEDOUT;
- }
- return 0;
+ return read_poll_timeout_atomic(oc_getreg, status,
+ (status & mask) == val,
+ 0, timeout_us, false,
+ i2c, reg);
}
/**
@@ -314,7 +308,7 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
* once we are here we expect to get the expected result immediately
* so if after 1ms we timeout then something is broken.
*/
- err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
+ err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, 1000);
if (err)
dev_warn(i2c->adap.dev.parent,
"%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
--
2.43.0On Tue, Mar 24, 2026 at 03:05:56PM +0100, Martin Aberer wrote:
> Replace the manual polling loop in ocores_wait() with the kernel helper
> read_poll_timeout_atomic(). This simplifies the code and ensures robust
> timeout handling. By using this helper, we avoid spurious timeout errors
> that could occur under high CPU load or preemption, as the macro handles
> timing and condition checks atomically.
It is not that it does it atomically, but that it always does a check
after the delay, even if the delay has taken us past the timeout.
Apart from that, and the other comments:
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
Hi,
On Fri, Mar 27, 2026 at 01:27:16AM +0100, Andrew Lunn wrote:
> On Tue, Mar 24, 2026 at 03:05:56PM +0100, Martin Aberer wrote:
> > Replace the manual polling loop in ocores_wait() with the kernel helper
> > read_poll_timeout_atomic(). This simplifies the code and ensures robust
> > timeout handling. By using this helper, we avoid spurious timeout errors
> > that could occur under high CPU load or preemption, as the macro handles
> > timing and condition checks atomically.
>
> It is not that it does it atomically, but that it always does a check
> after the delay, even if the delay has taken us past the timeout.
I took the liberty of changing the commit log to this:
i2c: ocores: Use read_poll_timeout_atomic to avoid false poll timeouts
Replace the manual polling loop in ocores_wait() with the kernel helper
read_poll_timeout_atomic(). This simplifies the code and ensures robust
timeout handling.
In particular, the helper guarantees a condition check after the
delay, even if the delay exceeds the timeout, avoiding spurious
timeout errors under load or preemption.
Let me know if it doesn't work.
I merged the patch to i2c/i2c-host.
Thanks,
Andi
> In particular, the helper guarantees a condition check after the
> delay, even if the delay exceeds the timeout, avoiding spurious
> timeout errors under load or preemption.
>
> Let me know if it doesn't work.
That is good, thanks.
Andrew
Hi, On Tue, Mar 24, 2026 at 03:05:56PM +0100, Martin Aberer wrote: > Replace the manual polling loop in ocores_wait() with the kernel helper > read_poll_timeout_atomic(). This simplifies the code and ensures robust > timeout handling. By using this helper, we avoid spurious timeout errors > that could occur under high CPU load or preemption, as the macro handles > timing and condition checks atomically. > > Signed-off-by: Martin Aberer <martin.aberer@bachmann.info> Please, next time don't send this as --in-reply-to, but please send it as a v2 with a proper changelog. Other than this, Christian, Andrew, anyone who wants to give this a look? Thanks, Andi
© 2016 - 2026 Red Hat, Inc.