Re: [PATCH] i2c: ocores: re-check status after poll timeout to avoid false errors

[Christian Gmeiner]

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@xxxxxxxxxxxxx>
> ---
>  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