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

From: Stefan Eichenberger <eichest@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: o.rempel@pengutronix.de, kernel@pengutronix.de,
	andi.shyti@kernel.org, shawnguo@kernel.org,
	s.hauer@pengutronix.de, festevam@gmail.com, jic23@kernel.org,
	lars@metafoo.de, nuno.sa@analog.com,
	u.kleine-koenig@pengutronix.de, marcelo.schmitt@analog.com,
	gnstark@salutedevices.com, francesco.dolcini@toradex.com,
	linux-i2c@vger.kernel.org, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	Stefan Eichenberger <stefan.eichenberger@toradex.com>
Subject: Re: [RFC PATCH] i2c: imx: avoid rescheduling when waiting for bus not busy
Date: Mon, 3 Jun 2024 09:43:35 +0200	[thread overview]
Message-ID: <Zl10J9H+8k0KXAPR@eichest-laptop> (raw)
In-Reply-To: <Zlnidi62gEWwdQ3U@smile.fi.intel.com>

On Fri, May 31, 2024 at 05:45:10PM +0300, Andy Shevchenko wrote:
> 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.
> 
> ...
> 
> > +			/*
> > +			 * 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();
> 
> Isn't there cond_resched() or so for such things?
> More info here: 494e46d08d35 ("airo: Replace in_atomic() usage.")

The problem would be that I have to disable preemption during the
transfer, then cond_resched would do nothing if I understand it
correctly. However, an I2C transfer @100kHz for 3 bytes takes at least
240us + overhead (e.g. waiting for the bus idle) which might end in a
close to ms ranage. This is what concerns me.

Regards,
Stefan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel