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

[Andrew Lunn]

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

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