Re: [RFC PATCH] i2c: imx: avoid rescheduling when waiting for bus not busy

[Stefan Eichenberger <eichest@gmail.com>]

Hi Oleksij,

On Sat, Jun 22, 2024 at 07:02:39AM +0200, Oleksij Rempel wrote:
> Hi Stefan,
> 
> On Fri, Jun 21, 2024 at 05:22:46PM +0200, Stefan Eichenberger wrote:
> > Hi Andi, Andrew, Wolfram, Oleksij,
> > 
> > After some internal discussion we still have some questions which are
> > blocking us from solving the issue.
> > 
> > On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > 
> > > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> > > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> > > bus is idle. The imx i2c driver will call schedule when waiting for the
> > > bus to become idle after switching to master mode. When the i2c
> > > controller switches to master mode it pulls SCL and SDA low, if the
> > > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> > > clocking, it will timeout and ignore all signals until the next start
> > > condition occurs (SCL and SDA low). This can occur when the system load
> > > is high and schedule returns after more than 25 ms.
> > > 
> > > This rfc tries to solve the problem by using a udelay for the first 10
> > > ms before calling schedule. This reduces the chance that we will
> > > reschedule. However, it is still theoretically possible for the problem
> > > to occur. To properly solve the problem, we would also need to disable
> > > interrupts during the transfer.
> > > 
> > > After some internal discussion, we see three possible solutions:
> > > 1. Use udelay as shown in this rfc and also disable the interrupts
> > >    during the transfer. This would solve the problem but disable the
> > >    interrupts. Also, we would have to re-enable the interrupts if the
> > >    timeout is longer than 1ms (TBD).
> > > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
> > >    timeout, we try again.
> > > 3. We use the suggested solution and accept that there is an edge case
> > >    where the timeout can happen.
> > > 
> > > There may be a better way to do this, which is why this is an RFC.
> > > 
> > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > ---
> > >  drivers/i2c/busses/i2c-imx.c | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > > index 3842e527116b7..179f8367490a5 100644
> > > --- a/drivers/i2c/busses/i2c-imx.c
> > > +++ b/drivers/i2c/busses/i2c-imx.c
> > > @@ -503,10 +503,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
> > >  				"<%s> I2C bus is busy\n", __func__);
> > >  			return -ETIMEDOUT;
> > >  		}
> > > -		if (atomic)
> > > +		if (atomic) {
> > >  			udelay(100);
> > > -		else
> > > -			schedule();
> > > +		} else {
> > > +			/*
> > > +			 * Avoid rescheduling in the first 10 ms to avoid
> > > +			 * timeouts for SMBus like devices
> > > +			 */
> > > +			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
> > > +				udelay(10);
> > > +			else
> > > +				schedule();
> > > +		}
> > >  	}
> > >  
> > >  	return 0;
> > > -- 
> > > 2.40.1
> > 
> > If we want to be sure that the ADS1015 I2C ADC will never timeout, we
> > would have to add a patch to disable preemption during transmission.
> > This would look like this:
> > 
> > @@ -1244,6 +1248,12 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
> >  	bool is_lastmsg = false;
> >  	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
> >  
> > +	/* If we are in SMBus mode we need to do the transfer atomically */
> > +	if (i2c_imx->smbus_mode) {
> > +		preempt_disable();
> > +		atomic = true;
> > +	}
> > +
> >  	/* Start I2C transfer */
> >  	result = i2c_imx_start(i2c_imx, atomic);
> >  	if (result) {
> > @@ -1320,6 +1330,9 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
> >  	if (i2c_imx->slave)
> >  		i2c_imx_slave_init(i2c_imx);
> >  
> > +	if (i2c_imx->smbus_mode)
> > +		preempt_enable();
> > +
> >  	return (result < 0) ? result : num;
> >  }
> > 
> > However, we are aware that disabling preemption is not a good idea. So
> > we were discussing how this is normally handled with SMBus devices? Is
> > it just expected that SMBus devices will timeout in rare cases?
> > 
> > For our use case, the problem would be solved if we could get rid of the
> > schedule call and replace it with a udelay. It seems that the i.MX8M
> > Mini I2C controller needs a few ms to clear the IBB flag. In the
> > reference manual, they write:
> > > I2C bus busy bit. Indicates the status of the bus. NOTE: When I2C is
> > > enabled (I2C_I2CR[IEN] = 1), it continuously polls the bus data (SDA)
> > > and clock (SCL) signals to determine a Start or Stop condition. Bus is
> > > idle. If a Stop signal is detected, IBB is cleared. Bus is busy. When
> > > Start is detected, IBB is set.
> > Unfortunately, it is not clear how often they poll. In our tests the
> > issue disappeard when we used udelay instead of usleep or schedule for
> > the first 10 ms.
> 
> I'm not really happy with this variant. It can be acceptable in some
> use cases, but may have not pleasant side effects on other. Since this
> is still valid case, I would prefer to have a UAPI to enable polling
> mode as optional optimization with a warning that it may affect latency
> on other corner of the system.
> 
> > Since we know we don't have a multi-master configuration, another way
> > would be to not test for IBB and just start the transfer. We saw that
> > other drivers use the multi-master device tree property to determine if
> > multi-master is supported. Here another quote from the reference manual:
> > > On a multimaster bus system, the busy bus (I2C_I2SR[IBB]) must be
> > > tested to determine whether the serial bus is free.
> > We assume this means it is not necessary to test for IBB if we know we
> > are in a single-master configuration.
> 
> I interpret this part of documentation in the same way, so it will be
> great if you can implement this option.

Perfect thanks a lot for the feedback. I will try to implement this
option and run some tests to make sure it will not break anything.

Best regards,
Stefan